fix(runtime): replace silent catches in overlay with console.warn#61
Merged
fix(runtime): replace silent catches in overlay with console.warn#61
Conversation
Contributor
There was a problem hiding this comment.
Findings
- [Major] Unbounded warning spam in 200ms reattach loop can become a log-amplification DoS in hostile iframe content —
console.warn(...)now runs on every failed listener operation insidereattach(), which executes every 200ms, so malicious/generated code that breaksaddEventListener/removeEventListenercan flood logs and degrade runtime responsiveness, evidencepackages/runtime/src/overlay.ts:103.
Suggested fix:var warned = Object.create(null); function warnOnce(key, err) { if (warned[key]) return; warned[key] = true; console.warn('[overlay] ' + key, err); } // in reattach loop try { document.removeEventListener(spec.evt, spec.fn, true); } catch (err) { warnOnce('removeEventListener failed for ' + spec.evt + ':', err); } try { document.addEventListener(spec.evt, spec.fn, true); } catch (err) { warnOnce('addEventListener failed for ' + spec.evt + ':', err); }
Summary
- Review mode: initial
- 1 issue found in modified lines.
docs/VISION.mdanddocs/PRINCIPLES.md: Not found in repo/docs.
Testing
- Not run (automation). Missing coverage for repeated listener-failure path to verify logging is rate-limited.
open-codesign Bot
| var spec = installs[i]; | ||
| try { document.removeEventListener(spec.evt, spec.fn, true); } catch (_) {} | ||
| try { document.addEventListener(spec.evt, spec.fn, true); } catch (_) {} | ||
| try { document.removeEventListener(spec.evt, spec.fn, true); } catch (err) { console.warn('[overlay] removeEventListener failed for ' + spec.evt + ':', err); } |
Contributor
There was a problem hiding this comment.
[Major] This catch now warns inside a function that runs every 200ms; if sandbox content breaks listener APIs, this can emit warnings continuously and degrade runtime (log-amplification DoS). Please gate this with warn-once or rate-limiting.
Suggested fix:
var warned = Object.create(null);
function warnOnce(key, err) {
if (warned[key]) return;
warned[key] = true;
console.warn('[overlay] ' + key, err);
}
try { document.removeEventListener(spec.evt, spec.fn, true); }
catch (err) { warnOnce('removeEventListener failed for ' + spec.evt + ':', err); }…gnostics The 8 catch blocks in the overlay script silently swallowed errors, making it hard to diagnose iframe sandbox issues (postMessage failures, listener attach failures, setInterval failures). Replaced each with console.warn emitting a contextual tag so problems surface in the sandbox iframe DevTools console without affecting overlay behaviour. Signed-off-by: hqhq1025 <1506751656@qq.com>
The overlay's 200ms setInterval reattach loop could flood the console when iframe content breaks add/removeEventListener (Codex Major on #61). Introduce a warnOnce helper keyed by error message so each unique failure warns at most once over the loop's lifetime. One-shot catches outside the loop keep their direct console.warn. Adds vitest coverage simulating 25 reattach ticks with throwing listeners and asserts warn count stays bounded by unique-key count, not tick count. Signed-off-by: hqhq1025 <1506751656@qq.com>
dbd3af6 to
d05f001
Compare
Contributor
There was a problem hiding this comment.
Findings
- [Minor] Silent fallback reintroduced in
warnOnce—console.warnfailures are swallowed withcatch (_) { /* noop */ }, which conflicts with the repo rule that errors must surface (UI or throw with context); this can hide diagnostics when console APIs are patched/broken, evidencepackages/runtime/src/overlay.ts:26.
Suggested fix:function warnOnce(key, err) { if (warned[key]) return; warned[key] = true; console.warn('[overlay] ' + key, err); }
Summary
- Review mode: follow-up after new commits
- 1 issue found in modified lines.
docs/VISION.mdanddocs/PRINCIPLES.md: Not found in repo/docs.
Testing
- Not run (automation)
open-codesign Bot
| function warnOnce(key, err) { | ||
| if (warned[key]) return; | ||
| warned[key] = true; | ||
| try { console.warn('[overlay] ' + key, err); } catch (_) { /* noop */ } |
Contributor
There was a problem hiding this comment.
[Minor] warnOnce currently swallows console.warn errors (catch (_) { /* noop */ }), reintroducing a silent fallback. Please let this throw (or surface via existing iframe error channel) so failures are observable with context.
Suggested fix:
function warnOnce(key, err) {
if (warned[key]) return;
warned[key] = true;
console.warn('[overlay] ' + key, err);
}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
catch (_) {}blocks inpackages/runtime/src/overlay.tsswallowed every error frompostMessage, listener attach/detach, andsetIntervalcalls — making sandbox iframe issues invisible.catch (err) { console.warn('[overlay] <context>:', err); }so failures surface in the iframe DevTools console without changing overlay behaviour.console.warnis the proper diagnostic channel inside the sandboxed renderer iframe (no IPC needed, doesn't escalate to IFRAME_ERROR loop).Locations covered (line numbers in original):
postMessage ELEMENT_SELECTED failedpostMessage IFRAME_ERROR (error) failedpostMessage IFRAME_ERROR (unhandledrejection) failedremoveEventListener failed for <evt>addEventListener failed for <evt>attach window error listener failedattach unhandledrejection listener failedsetInterval reattach failedCompatibility / Upgradeability / No bloat / Elegance
Test plan
pnpm typecheckpnpm lint(exit 0; pre-existing warnings unrelated)pnpm --filter @open-codesign/runtime test— 3/3 passed