feat(proxy): auto-proxy internal addon endpoints and subtitle URLs#1029
Open
IbbyLabs wants to merge 1 commit into
Open
feat(proxy): auto-proxy internal addon endpoints and subtitle URLs#1029IbbyLabs wants to merge 1 commit into
IbbyLabs wants to merge 1 commit into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds internal-endpoint detection for proxy decisions. Core stream and subtitle handling now use it to widen proxying, and frontend addon install/edit flows now derive and apply proxy configuration automatically for internal addon manifests. ChangesAuto-proxy for internal addon endpoints
Estimated code review effort: 4 (Complex) | ~50 minutes Suggested reviewers: Viren070 Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
packages/frontend/src/components/menu/addons/_components/my-addons.tsx (1)
1026-1068:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSame
autoEnabledProxytiming issue as inindex.tsx.The
autoEnabledProxyvariable is assigned inside thesetUserDatacallback (line 1052) but used outside for toast display (line 1059). This has the same potential race condition as the add/edit flows inindex.tsx.Apply the same fix pattern: compute
autoEnabledProxysynchronously before callingsetUserData.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/frontend/src/components/menu/addons/_components/my-addons.tsx` around lines 1026 - 1068, The variable `autoEnabledProxy` is being assigned inside the `setUserData` callback function, but then used immediately after to conditionally display toast messages, which creates a race condition since state updates are asynchronous. Move the `applyInternalAddonProxyConfig` call outside the `setUserData` callback and compute `autoEnabledProxy` synchronously before calling `setUserData`, then pass the computed result to the state setter to ensure the variable is available for the toast logic that checks if `autoEnabledProxy` is true.packages/frontend/src/components/menu/addons/index.tsx (1)
270-305:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSame
autoEnabledProxytiming issue in edit flow.The edit flow has the same potential race condition as the add flow -
autoEnabledProxyis assigned inside thesetUserDatacallback but used outside it for toast display.Apply the same fix pattern as suggested for the add flow.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/frontend/src/components/menu/addons/index.tsx` around lines 270 - 305, The autoEnabledProxy variable is assigned inside the setUserData state callback but used outside it for conditional toast messages, creating a race condition. Restructure the code to determine autoEnabledProxy before the setUserData call by extracting the autoProxyResult assignment outside the callback. Create the autoProxyResult from applyInternalAddonProxyConfig before calling setUserData, then use that same result both for the state update and for the subsequent conditional toast messages that check autoEnabledProxy.
🧹 Nitpick comments (1)
packages/frontend/src/components/menu/addons/_components/proxy-auto.ts (1)
31-56: ⚖️ Poor tradeoffConsider extracting shared
isInternalEndpointlogic to avoid duplication.This function duplicates the logic from
packages/core/src/utils/http.ts. While the frontend version omits the bootstrap URL checks (which makes sense as those configs aren't available client-side), the heuristics should stay in sync. The IPv6 detection issues noted in the core version (false positives forfacebook.com,fdroid.org) also apply here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/frontend/src/components/menu/addons/_components/proxy-auto.ts` around lines 31 - 56, The isInternalEndpoint function duplicates logic from the core version in packages/core/src/utils/http.ts. Extract the common hostname checking heuristics (localhost, IPv4 private ranges, IPv6 detection, etc.) into a shared utility that both the frontend and core versions can reference to keep them in sync. Since frontend and core have different contexts, create a shared utility file that both packages can import from. Additionally, review and fix the IPv6 detection logic (hostname.startsWith('fc') and hostname.startsWith('fd')) that produces false positives for domains like facebook.com and fdroid.org by making the patterns more strict to properly detect link-local and unique-local IPv6 addresses instead of catching common domain patterns.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/utils/http.ts`:
- Around line 316-321: The IPv6 address detection logic has two issues: first,
the checks for ::1 and fe80: need to account for bracket-wrapped format that
url.hostname may return (e.g., [::1] instead of ::1), and second, the string
prefix checks for fc and fd are too broad and will incorrectly match public
domains like facebook.com and fdroid.org as internal endpoints. Fix this by
either stripping brackets before checking IPv6 patterns, or adjusting the checks
to match the full bracket-wrapped format; additionally, make the ULA prefix
checks more specific by validating that they match actual IPv6 ULA addresses
(which follow the pattern of 4 hexadecimal characters followed by a colon, e.g.,
fc00: or fd00:) rather than just checking if the string starts with these
characters.
In `@packages/frontend/src/components/menu/addons/_components/proxy-auto.ts`:
- Line 1: Replace the relative path import in the import statement at the top of
proxy-auto.ts file that imports from the core package constants. Instead of
using the relative path traversal
(../../../../../../../../core/src/utils/constants), use the workspace import
alias `@aiostreams/core` to import the constants module. Update all usages of the
imported constants throughout the file to ensure they remain compatible with the
new import path.
In `@packages/frontend/src/components/menu/addons/index.tsx`:
- Around line 236-267: The variable autoEnabledProxy is being assigned inside
the setUserData callback but the toast logic that depends on it executes
synchronously afterward, causing stale closure values. Compute autoEnabledProxy
before calling setUserData by extracting the logic: first call
applyInternalAddonProxyConfig to get the autoProxyResult and extract
autoEnabledProxy, then use that result when calling setUserData and when
evaluating the toast conditions. This ensures the toast logic uses the actual
computed value rather than the initial false value.
---
Duplicate comments:
In `@packages/frontend/src/components/menu/addons/_components/my-addons.tsx`:
- Around line 1026-1068: The variable `autoEnabledProxy` is being assigned
inside the `setUserData` callback function, but then used immediately after to
conditionally display toast messages, which creates a race condition since state
updates are asynchronous. Move the `applyInternalAddonProxyConfig` call outside
the `setUserData` callback and compute `autoEnabledProxy` synchronously before
calling `setUserData`, then pass the computed result to the state setter to
ensure the variable is available for the toast logic that checks if
`autoEnabledProxy` is true.
In `@packages/frontend/src/components/menu/addons/index.tsx`:
- Around line 270-305: The autoEnabledProxy variable is assigned inside the
setUserData state callback but used outside it for conditional toast messages,
creating a race condition. Restructure the code to determine autoEnabledProxy
before the setUserData call by extracting the autoProxyResult assignment outside
the callback. Create the autoProxyResult from applyInternalAddonProxyConfig
before calling setUserData, then use that same result both for the state update
and for the subsequent conditional toast messages that check autoEnabledProxy.
---
Nitpick comments:
In `@packages/frontend/src/components/menu/addons/_components/proxy-auto.ts`:
- Around line 31-56: The isInternalEndpoint function duplicates logic from the
core version in packages/core/src/utils/http.ts. Extract the common hostname
checking heuristics (localhost, IPv4 private ranges, IPv6 detection, etc.) into
a shared utility that both the frontend and core versions can reference to keep
them in sync. Since frontend and core have different contexts, create a shared
utility file that both packages can import from. Additionally, review and fix
the IPv6 detection logic (hostname.startsWith('fc') and
hostname.startsWith('fd')) that produces false positives for domains like
facebook.com and fdroid.org by making the patterns more strict to properly
detect link-local and unique-local IPv6 addresses instead of catching common
domain patterns.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6fc80dc7-303f-4cd4-8559-92b11b0167c9
📒 Files selected for processing (6)
packages/core/src/main/resources.tspackages/core/src/streams/proxifier.tspackages/core/src/utils/http.tspackages/frontend/src/components/menu/addons/_components/my-addons.tsxpackages/frontend/src/components/menu/addons/_components/proxy-auto.tspackages/frontend/src/components/menu/addons/index.tsx
IbbyLabs
force-pushed
the
feat/auto-proxy-http
branch
from
f0411bb to
0752166
Compare
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/frontend/src/components/menu/addons/_components/my-addons.tsx (1)
1015-1025:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not validate auto-proxied internal manifests with a browser fetch.
Line 1017 still fetches the container/internal URL directly before the proxy update at Lines 1045-1051. For hostnames only reachable inside the self-hosted network, this fails and returns early, so the addon is never added to
proxiedAddons. Skip this client-side fetch whenautoProxyDecision.shouldAutoProxyis true, or validate through the backend/proxy instead.Proposed fix
- try { - setLoading(true); - const response = await fetch(std); - if (!response.ok) - throw new Error(`${response.status} ${response.statusText}`); - await response.json(); - } catch (error: any) { - toast.error(`Failed to fetch or parse manifest: ${error.message}`); - setLoading(false); - return; - } + setLoading(true); + if (!autoProxyDecision.shouldAutoProxy) { + try { + const response = await fetch(std); + if (!response.ok) + throw new Error(`${response.status} ${response.statusText}`); + await response.json(); + } catch (error: any) { + toast.error(`Failed to fetch or parse manifest: ${error.message}`); + setLoading(false); + return; + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/frontend/src/components/menu/addons/_components/my-addons.tsx` around lines 1015 - 1025, The fetch call to validate the manifest at line 1017 (within the try block fetching from `std`) needs to be skipped when `autoProxyDecision.shouldAutoProxy` is true, since internal container URLs are not reachable from the client browser. Add a conditional check before the fetch statement to skip the client-side validation when auto-proxy is enabled, allowing the backend/proxy to handle the validation instead after the proxy configuration is applied at lines 1045-1051. This prevents early returns that block the addon from being added to `proxiedAddons`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/utils/http.ts`:
- Around line 299-312: The regex patterns checking for private IPv4 ranges
(127., 10., 192.168., 172.16-31., 169.254.) are matching any hostname that
starts with these strings, which incorrectly classifies DNS names like
10.example.com as private addresses. Parse and validate that the hostname is a
complete IPv4 literal (matching the full format of four dot-separated numeric
octets) before applying these private range checks. This ensures only actual
IPv4 addresses in private ranges are identified as internal, while DNS names
that happen to start with these patterns are not misclassified.
In `@packages/frontend/src/components/menu/addons/_components/my-addons.tsx`:
- Around line 1026-1051: The current implementation builds nextUserData from
userData that was captured before an asynchronous fetch operation, and then
ignores the setter's previous value when calling setUserData. This can overwrite
addon or proxy changes made while the fetch was pending. Move the entire block
of logic that finds currentPreset, builds baseNextUserData, applies
autoProxyDecision, and constructs nextUserData (lines 1026-1050) into a
functional update callback passed to setUserData. This ensures that the update
operates on the latest state at the time of the update rather than a stale
snapshot, preventing concurrent edits from being lost.
- Around line 36-39: The import statement for the proxy-auto module is missing
the required `.js` extension in the relative import path. Update the import
statement where the modules applyInternalAddonProxyConfig and
shouldAutoProxyInternalAddon are imported from './proxy-auto' to
'./proxy-auto.js' to comply with TypeScript NodeNext module resolution
requirements.
In `@packages/frontend/src/components/menu/addons/_components/proxy-auto.ts`:
- Around line 44-48: The regex patterns in the private IP range checks are
matching DNS hostnames that merely start with private range prefixes (like
10.example.com or 192.168.example.com) rather than actual private IP addresses.
Update each regex pattern in the conditions starting with /^(127\.)/, /^(10\.)/,
/^(192\.168\.)/, /^172\.(1[6-9]|2[0-9]|3[0-1])\./, and /^169\.254\./ to ensure
they match only complete IPv4 addresses in those private ranges, not arbitrary
hostnames with matching prefixes. This requires making the patterns stricter by
ensuring that after the prefix, only numeric octets and dots follow until the
end of the string, preventing false matches on DNS names.
---
Outside diff comments:
In `@packages/frontend/src/components/menu/addons/_components/my-addons.tsx`:
- Around line 1015-1025: The fetch call to validate the manifest at line 1017
(within the try block fetching from `std`) needs to be skipped when
`autoProxyDecision.shouldAutoProxy` is true, since internal container URLs are
not reachable from the client browser. Add a conditional check before the fetch
statement to skip the client-side validation when auto-proxy is enabled,
allowing the backend/proxy to handle the validation instead after the proxy
configuration is applied at lines 1045-1051. This prevents early returns that
block the addon from being added to `proxiedAddons`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 33a32cd3-87e7-4e6d-8c6d-4935b61f691d
📒 Files selected for processing (6)
packages/core/src/main/resources.tspackages/core/src/streams/proxifier.tspackages/core/src/utils/http.tspackages/frontend/src/components/menu/addons/_components/my-addons.tsxpackages/frontend/src/components/menu/addons/_components/proxy-auto.tspackages/frontend/src/components/menu/addons/index.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/streams/proxifier.ts
- packages/frontend/src/components/menu/addons/index.tsx
- packages/core/src/main/resources.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/frontend/src/components/menu/addons/_components/my-addons.tsx`:
- Around line 1055-1059: The toast message in the autoProxyDecision conditional
is displaying a single generic message when it should differentiate between two
scenarios: whether the proxy was auto-enabled (credentials present) or
auto-targeted (requires credential configuration). Compute an autoEnabledProxy
boolean value before calling setUserData to determine which case applies, then
display different toast messages using toast.info based on this computed value
to match the messaging pattern used in index.tsx, ensuring users understand if
further proxy configuration is needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bf4c1789-b26b-418f-9377-428d94620dee
📒 Files selected for processing (3)
packages/core/src/utils/http.tspackages/frontend/src/components/menu/addons/_components/my-addons.tsxpackages/frontend/src/components/menu/addons/_components/proxy-auto.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/utils/http.ts
- packages/frontend/src/components/menu/addons/_components/proxy-auto.ts
Contributor
Author
|
@Viren070 whenever you get a chance to look at this, I would appreciate your opinion |
IbbyLabs
force-pushed
the
feat/auto-proxy-http
branch
from
f24414d to
31d2c90
Compare
Auto-route addon stream and subtitle URLs whose host is an internal container hostname or private IP (e.g. http://nzbdav:8080) through the configured proxy, since an external player can't reach those. Also lets subtitle URLs be proxied on demand via the `subtitle` proxied-service, mirroring stream proxying.
IbbyLabs
force-pushed
the
feat/auto-proxy-http
branch
from
31d2c90 to
db49874
Compare
IbbyLabs
changed the title
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.
Two related proxy changes:
http://nzbdav:8080) through the configured proxy. An external player can't reach a container host or private address, so these are proxied regardless of the per-addon/per-service filters.subtitleproxied-service, selectable in the proxy settings and documented for theDEFAULT_PROXY_PROXIED_SERVICES/FORCE_PROXY_PROXIED_SERVICESenv vars. Mirrors how stream proxying is opted into.Supersedes #1020 (its subtitle-URL proxying is included here).