fix(security): consolidate all outbound requests behind one hardened gate#290
Merged
Conversation
Deploying podnotes with
|
| Latest commit: |
881e961
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3b0344ca.podnotes.pages.dev |
| Branch Preview URL: | https://fix-harden-fetch-boundary.podnotes.pages.dev |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9596763e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…gate Route every fetch call site (episode download, streaming, chapters, iTunes search, Deepgram diarization, feed documents, universal links) through a single requestWithTimeout gate that classifies the target, bounds request and response sizes, applies timeouts, and returns redacted typed errors. Fold LiteralTargetClassifier's stricter address classification into assertFetchableUrl (full IANA special-purpose IPv4/IPv6 registries, canonical-form-only literal parsing) and delete the module - the last remnant of the removed capability architecture. Replace ad hoc per-site guards with one chokepoint so new call sites cannot bypass classification. requestUrl's documented limitations (no per-hop redirect inspection, no native cancellation, buffered bodies) remain the accepted design ceiling per #288. Concurrent downloads to the same destination now dedupe on the normalized destination path with ownership validation instead of racing.
chhoumann
force-pushed
the
fix/harden-fetch-boundary
branch
from
e959676 to
c6d2fde
Compare
… path The fast path threw a destination collision when a foreign file occupied the provisional URL-extension path, but that path is only a guess - the probe can resolve a different final extension whose destination is free. The fast path now returns early only for the registered owner and otherwise falls through; the collision check runs on the resolved final path, where it already existed. Costs one Range probe before the error in the true-collision case. (Codex review finding on #290.)
This was referenced Jul 11, 2026
chhoumann
added a commit
that referenced
this pull request
Jul 11, 2026
Review follow-up: sanitizePortableResourceUrl blocks literal private and credentialed targets but a public hostname that DNS-resolves to a private address still passes - the same ceiling #290 documents, unfixable without DNS resolution the render layer can't do. Document it rather than imply complete protection.
chhoumann
added a commit
that referenced
this pull request
Jul 11, 2026
…sts (#297) * fix(security): keep generated notes from making browser-managed requests Feed-controlled show-notes HTML could embed active media elements (img, audio, video, source, iframe, object, embed, link) whose requests fire when a generated note renders - reaching feed-selected local/private targets without ever passing PodNotes' network gate. Markdown image syntax in feed text formed the same live embeds after conversion. feedHtmlToMarkdown now strips active elements before htmlToMarkdown (keeping image alt text), neutralizes Markdown embeds afterwards with balanced escaping, and the portable resource tags ({{artwork}}, {{episodeartwork}}, {{feedartwork}}, {{stream}}) emit only public http(s) URLs without embedded credentials - anything else renders as an empty string. Deliberate UX change: show-note images render as their alt text instead of live embeds; a note render is an ambient fetch the reader never chose, unlike clicking a link (links are preserved). Ported from the salvaged pre-#288 hardening pass (salvage branch 0bf89a4) onto the current gate API. * docs: name the DNS-rebinding residual on portable resource URLs Review follow-up: sanitizePortableResourceUrl blocks literal private and credentialed targets but a public hostname that DNS-resolves to a private address still passes - the same ceiling #290 documents, unfixable without DNS resolution the render layer can't do. Document it rather than imply complete protection.
chhoumann
added a commit
that referenced
this pull request
Jul 11, 2026
* fix(player): show an error state when media fails to load isLoading was cleared only by loadedmetadata, and the media elements had no error handler - a blocked, dead, or unreachable stream URL left the player spinning forever. Both media elements now handle error by replacing the spinner with a visible error overlay; the state resets on the next episode switch. Found while gating all outbound requests (#290): a refused stream URL looked identical to a slow one. * fix(player): guard persistence separately from the loading spinner Addressing review: onMediaError cleared isLoading, but isLoading also gated persistPlaybackPosition, so a failed load (which never restores the saved position) could let a later pause/destroy write the pre-restore 0 over the stored resume point. Split the concerns: isLoading stays a pure display flag; a dedicated playbackRestored guard, set only by a real metadata load, gates persistence. Regression test: a failed load followed by unmount keeps the saved position intact.
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
1 participant
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.
Routes every outbound request PodNotes makes - episode download, streaming, chapter fetch, iTunes search, Deepgram diarization, feed documents, and universal-link resolution - through a single
requestWithTimeoutgate insrc/utility/networkRequest.ts. The gate classifies the target, bounds request and response sizes, applies timeouts and abort support, and returns redacted typed errors that never contain the raw URL.Why: podcast feeds are hostile input, and until now each call site independently decided whether to invoke the URL guard. This makes "every request is classified" a structural property: a future call site that skips the gate has no
requestUrlimport to reach for. This is the consolidation follow-up to #288 - PR 2 of the three-PR plan that replaced the capability architecture.What folds in:
LiteralTargetClassifier(the last preserved remnant of the deleted capability stack) is absorbed intoassertFetchableUrland removed. The guard now covers the full IANA special-purpose IPv4/IPv6 registries, parses literals only in canonical form (the WHATWG parser normalizes obfuscated encodings first), rejects credentials in URLs, and enforces target-size bounds.What this deliberately does not do: chase guarantees
requestUrlcannot give. No per-hop redirect inspection, no DNS pinning, no native cancellation - those are the accepted design ceiling documented in #288. Timeouts are logical (the buffered transfer is observed, not killed).Also: concurrent downloads that resolve to the same destination file now dedupe on the normalized destination path with ownership validation against the downloads store, replacing the previous unguarded race.
Review focus:
assertFetchableUrl.tsclassification completeness (172 focused tests)networkRequest.tssettled-once timeout/abort semanticsGates: lint, format, typecheck, build, full suite green locally. Bundle 447.37 kB (-38 kB vs pre-#288 baseline).
Merge order: holds as draft until #287/#289 land, then rebases onto master.