Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 68 additions & 2 deletions src/adapters/inbound/ui/SidecarPanelAdapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -239,8 +239,8 @@ export class SidecarPanelAdapter {
await this.handleOpenHNStory(message.url);
break;
case 'openHNStoryInPanel':
// Use new content view system instead of separate panel
this.panelStateManager?.openContentView(message.url, message.title);
// Check if URL can be embedded, otherwise open externally
await this.handleOpenHNStoryInPanel(message.url, message.title);
break;
case 'openContentView':
this.panelStateManager?.openContentView(message.url, message.title);
Expand Down Expand Up @@ -756,6 +756,72 @@ export class SidecarPanelAdapter {
await vscode.env.openExternal(vscode.Uri.parse(hnUrl));
}

/**
* Check if a URL can be embedded in an iframe
* Returns true if the URL allows iframe embedding, false otherwise
*/
private async checkIframeEmbeddable(url: string): Promise<boolean> {
try {
// Use native fetch with HEAD request to check headers
const controller = new AbortController();
const timeoutId = setTimeout(() => controller.abort(), 5000);

const response = await fetch(url, {
method: 'HEAD',
signal: controller.signal,
redirect: 'follow',
});

clearTimeout(timeoutId);

// Check X-Frame-Options header
const xFrameOptions = response.headers.get('x-frame-options');
if (xFrameOptions) {
const value = xFrameOptions.toLowerCase();
if (value === 'deny' || value === 'sameorigin') {
return false;
}
}

// Check Content-Security-Policy frame-ancestors directive
const csp = response.headers.get('content-security-policy');
if (csp) {
const frameAncestorsMatch = csp.match(/frame-ancestors\s+([^;]+)/i);
if (frameAncestorsMatch) {
const ancestors = frameAncestorsMatch[1].toLowerCase().trim();
// 'none' or 'self' means no external embedding
if (ancestors === "'none'" || ancestors === "'self'") {
return false;
}
}
}

return true;
} catch {
// On error (timeout, network issues), assume embeddable and let iframe handle it
return true;
Comment on lines +801 to +802
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The catch block currently assumes a URL is embeddable if the fetch request fails (e.g., due to a timeout). This is an optimistic approach that can lead to a poor user experience: the user waits for the check to time out, then the UI attempts to load the URL in an iframe, which might also fail, resulting in a blank panel.

A safer approach would be to assume the URL is not embeddable on error. This would redirect to an external browser, which is a better fallback than a blank panel, preventing the user from seeing a blank, non-functional view.

Suggested change
// On error (timeout, network issues), assume embeddable and let iframe handle it
return true;
// On error (timeout, network issues), assume not embeddable and open externally
return false;

}
}

/**
* Handle opening HN story in panel or external browser
* Checks if the URL can be embedded in iframe, opens externally if not
*/
private async handleOpenHNStoryInPanel(url: string, title: string): Promise<void> {
if (!url) return;

const canEmbed = await this.checkIframeEmbeddable(url);
if (canEmbed) {
this.panelStateManager?.openContentView(url, title);
} else {
// Open in external browser and show notification
await vscode.env.openExternal(vscode.Uri.parse(url));
vscode.window.showInformationMessage(
`"${title}" opened in browser (site blocks iframe embedding)`
);
}
}

private escapeHtml(text: string): string {
return text
.replace(/&/g, '&amp;')
Expand Down
86 changes: 80 additions & 6 deletions src/adapters/inbound/ui/webview/core/App.ts
Original file line number Diff line number Diff line change
Expand Up @@ -828,22 +828,96 @@ function renderContentViewState(contentView: ContentView): void {
}

if (iframe) {
// Track if content loaded successfully
let loadSuccessful = false;
let loadTimeout: ReturnType<typeof setTimeout> | null = null;

const showError = () => {
const loading = document.getElementById('content-loading');
const error = document.getElementById('content-error');
if (loading) loading.classList.add('hidden');
if (error) error.classList.remove('hidden');
iframe.style.display = 'none';
};

const showContent = () => {
loadSuccessful = true;
if (loadTimeout) {
clearTimeout(loadTimeout);
loadTimeout = null;
}
const loading = document.getElementById('content-loading');
if (loading) loading.classList.add('hidden');
iframe.style.display = 'block';
};

// Set a timeout to detect if iframe fails to load (X-Frame-Options, CSP blocks)
// Many sites block iframe embedding but don't trigger error events
loadTimeout = setTimeout(() => {
if (!loadSuccessful) {
// After timeout, check if iframe has any accessible content
try {
// Try to access iframe content - will throw for blocked/cross-origin frames
const doc = iframe.contentDocument;
// If we can access and it's empty or about:blank, likely blocked
if (doc && (doc.body?.innerHTML === '' || doc.URL === 'about:blank')) {
showError();
return;
}
// If accessible and has content, show it
if (doc && doc.body?.innerHTML) {
showContent();
return;
}
// Cross-origin but might be loaded - give benefit of doubt
showContent();
} catch {
// Cross-origin access denied - this is normal for loaded external sites
// The iframe might still be showing content, so show it
showContent();
}
}
}, 5000);

iframe.addEventListener(
'load',
() => {
const loading = document.getElementById('content-loading');
if (loading) loading.classList.add('hidden');
// iframe loaded - but might be empty if blocked by X-Frame-Options
// Check if we can detect empty content
setTimeout(() => {
try {
const doc = iframe.contentDocument;
// If we can access the document and it's essentially empty, likely blocked
if (doc) {
const bodyContent = doc.body?.innerHTML?.trim() || '';
const hasContent = bodyContent.length > 0 && doc.URL !== 'about:blank';
if (hasContent) {
showContent();
} else {
// Empty content likely means blocked
showError();
}
} else {
// Can't access document (cross-origin) - assume loaded successfully
showContent();
}
} catch {
// Cross-origin access denied - iframe is loaded with external content
showContent();
}
}, 100);
},
{ signal: getSignal() }
);

iframe.addEventListener(
'error',
() => {
const loading = document.getElementById('content-loading');
const error = document.getElementById('content-error');
if (loading) loading.classList.add('hidden');
if (error) error.classList.remove('hidden');
if (loadTimeout) {
clearTimeout(loadTimeout);
loadTimeout = null;
}
showError();
},
{ signal: getSignal() }
);
Comment on lines +831 to 923
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This new client-side logic for detecting blocked iframes is quite complex and has a couple of issues:

  1. Flawed Cross-Origin Detection: For a cross-origin iframe blocked by X-Frame-Options, the load event typically still fires. Your logic then tries to access iframe.contentDocument, which throws a cross-origin error. The catch block then optimistically calls showContent(), resulting in a blank iframe being shown to the user. This unfortunately doesn't solve the problem this fallback is intended to address.

  2. Code Duplication: The content inspection logic within the 5-second setTimeout (lines 859-878) is nearly identical to the logic inside the load event handler (lines 888-907). This could be refactored into a shared helper function to improve maintainability.

Given that the new server-side pre-check is the most reliable mechanism, you might consider simplifying or removing this complex client-side fallback. The current implementation adds significant complexity without reliably handling the primary cross-origin failure case.

Expand Down