Add proxySessionId to provider params for forwarding requests to same…#59
Add proxySessionId to provider params for forwarding requests to same…#59adiwajshing merged 5 commits intomainfrom
Conversation
WalkthroughAdds an optional proxySessionId (string) across provider schemas, client requests, server tunnel flows, and tests; it is template-resolved and validated by the HTTP provider, propagated in CreateTunnelRequest, enforced in claim/create handlers, and used when constructing proxy URLs. Changes
sequenceDiagram
participant Client
participant HTTPProvider
participant Server
participant ProxyGW as ProxyGateway
rect rgb(245,250,255)
Note over Client,HTTPProvider: Client supplies provider params (may include proxySessionId template)
Client->>HTTPProvider: resolve params (including proxySessionId)
HTTPProvider->>HTTPProvider: extract & replace template values
HTTPProvider->>HTTPProvider: validate via isValidProxySessionId
HTTPProvider-->>Client: resolved provider params
end
rect rgb(255,250,240)
Note over Client,Server: Create tunnel using resolved proxySessionId
Client->>Server: CreateTunnel(createRequest with proxySessionId)
Server->>Server: validate createRequest (including proxySessionId), label APM
alt proxy URL available
Server->>ProxyGW: open connection using proxy URL with session suffix (proxySessionId)
ProxyGW-->>Server: connection established (proxy IP)
else direct connect
Server->>Server: connect directly (no proxy session)
end
Server-->>Client: tunnel ready
end
rect rgb(240,255,240)
Note over Client,ProxyGW: Subsequent create with same proxySessionId
Client->>Server: CreateTunnel(same proxySessionId)
Server->>ProxyGW: reuse session (same proxy IP)
ProxyGW-->>Client: response routed via same proxy IP
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)src/tests/tcp-tunnel.test.ts (5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (4)
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}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/tests/rpc-tunnel.test.ts (1)
180-237: Remove trailing semicolons to satisfy lint rulesThe added
await assert.rejectsblocks end with);on Line 208 and Line 237, whereas the rest of this file uses a no‑semicolon style. Static analysis is flagging these as “extra semicolon”.You can fix this by dropping the semicolons:
- ) - ); + ) + ) @@ - ) - }); + ) + })The test logic for geoLocation and proxySessionId error propagation otherwise looks good.
src/server/tunnels/make-tcp-tunnel.ts (2)
192-214: Proxy routing condition now breaks existing geoLocation-only flowsThis block currently does:
if((proxySessionId || geoLocation) && !HTTPS_PROXY_URL) { // warn, then zero out both } if(!geoLocation || !proxySessionId) { socket.connect({ host, port }) return socket }This requires both geoLocation and proxySessionId to be truthy to go through the HTTPS proxy. Previously, geoLocation alone was sufficient to trigger proxying. As a result, any client that sets only
geoLocation(and noproxySessionId) will now fall back to a direct TCP connection without proxying, changing behavior in a non-obvious way.For backwards-compatible semantics:
- Use the proxy whenever a non-empty geoLocation is provided (as before).
- Treat
proxySessionIdas an optional enhancement on top of that.Concretely:
- if(!geoLocation || !proxySessionId) { - socket.connect({ host, port }) - return socket - } + // If no geoLocation is specified, connect directly without proxy. + if(!geoLocation) { + socket.connect({ host, port }) + return socket + }Then adjust the validation below (see next comment) so proxySessionId is optional.
216-236: Make proxySessionId optional and avoid rejecting empty valuesCurrently proxySessionId is validated unconditionally:
if(!isValidProxySessionId(proxySessionId)) { throw AttestorError.badRequest( `proxySessionId "${proxySessionId}" is invalid. ...`, { proxySessionId } ) }Given callers pass
proxySessionId: request.proxySessionId || '', an empty string is used to mean “no session id”. With the unconditional check,''is treated as invalid and will 400 even when geoLocation is valid.To treat proxySessionId as optional and allow “no session” semantics:
- if(!isValidProxySessionId(proxySessionId)) { + if(proxySessionId && !isValidProxySessionId(proxySessionId)) { throw AttestorError.badRequest( `proxySessionId "${proxySessionId}" is invalid. Must be a lowercase alphanumeric string of length 8-14 characters. eg. "mystring12345", "something1234".`, { proxySessionId } ) }Combined with the previous suggestion (proxy on any non-empty geoLocation), this preserves existing geoLocation-only behavior while enforcing a well-formed session id when one is provided.
🧹 Nitpick comments (1)
src/server/handlers/createTunnel.ts (1)
24-31: APM label for proxySessionId is useful; optionally guard emptiesAdding
sessionTx?.setLabel('proxySessionId', opts.proxySessionId)is a good observability hook for correlating tunnels by proxy session.If you want to avoid high‑cardinality labels that are often empty, you could optionally guard it:
if (opts.proxySessionId) { sessionTx?.setLabel('proxySessionId', opts.proxySessionId) }But the current behavior is functionally fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
proto/api.proto(1 hunks)provider-schemas/http/parameters.yaml(1 hunks)src/client/create-claim.ts(2 hunks)src/client/tunnels/make-rpc-tls-tunnel.ts(1 hunks)src/proto/api.ts(8 hunks)src/proto/tee-bundle.ts(1 hunks)src/providers/http/index.ts(4 hunks)src/server/handlers/claimTunnel.ts(1 hunks)src/server/handlers/createTunnel.ts(1 hunks)src/server/tunnels/make-tcp-tunnel.ts(7 hunks)src/server/utils/proxy-session.ts(1 hunks)src/tests/http-provider-utils.test.ts(6 hunks)src/tests/rpc-tunnel.test.ts(3 hunks)src/tests/tcp-tunnel.test.ts(3 hunks)src/types/providers.gen.ts(2 hunks)src/types/providers.ts(1 hunks)src/types/tunnel.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
src/types/tunnel.ts (1)
src/proto/api.ts (2)
CreateTunnelRequest(423-452)CreateTunnelRequest(1407-1525)
src/providers/http/index.ts (1)
src/server/utils/proxy-session.ts (1)
isValidProxySessionId(1-3)
src/server/tunnels/make-tcp-tunnel.ts (4)
src/providers/http/index.ts (2)
proxySessionId(57-61)geoLocation(52-56)src/utils/logger.ts (1)
logger(10-10)src/server/utils/proxy-session.ts (1)
isValidProxySessionId(1-3)src/utils/error.ts (1)
AttestorError(10-66)
src/tests/tcp-tunnel.test.ts (5)
src/providers/http/index.ts (1)
proxySessionId(57-61)src/server/tunnels/make-tcp-tunnel.ts (1)
makeTcpTunnel(41-102)src/utils/error.ts (1)
AttestorError(10-66)src/utils/http-parser.ts (1)
makeHttpResponseParser(52-232)src/utils/generics.ts (2)
strToUint8Array(41-43)uint8ArrayToStr(34-36)
src/client/create-claim.ts (2)
src/providers/http/index.ts (1)
proxySessionId(57-61)src/utils/generics.ts (1)
getProviderValue(145-150)
src/tests/http-provider-utils.test.ts (3)
src/utils/generics.ts (1)
getProviderValue(145-150)src/types/providers.ts (1)
ProviderParams(27-27)src/providers/http/index.ts (1)
proxySessionId(57-61)
🪛 GitHub Actions: Test
src/providers/http/index.ts
[error] 1-1: ESLint: Run autofix to sort imports (simple-import-sort/imports).
🪛 GitHub Check: test
src/server/utils/proxy-session.ts
[failure] 2-2:
Extra semicolon
[failure] 2-2:
Expected indentation of 1 tab but found 4 spaces
src/tests/rpc-tunnel.test.ts
[failure] 237-237:
Extra semicolon
[failure] 208-208:
Extra semicolon
src/providers/http/index.ts
[failure] 698-698:
Expected blank line before this statement
src/server/tunnels/make-tcp-tunnel.ts
[failure] 13-13:
import statements should have an absolute path
[failure] 234-234:
Unexpected space(s) after "if"
src/tests/tcp-tunnel.test.ts
[failure] 12-12:
Unexpected space(s) after "for"
🔇 Additional comments (11)
src/client/tunnels/make-rpc-tls-tunnel.ts (1)
76-85: proxySessionId propagation into CreateTunnelRequest looks correctUsing
proxySessionId: request.proxySessionId || ''ensures the field is always present and normalized to""when absent, matchingCreateTunnelRequestdefaults and keeping server-side equality checks consistent. No issues from this change.src/types/tunnel.ts (1)
22-25: TCPSocketProperties now correctly reflects proxySessionIdIncluding
'proxySessionId'in thePick<CreateTunnelRequest, ...>keepstunnel.createRequestaligned with the proto type and supports the additional equality check inclaimTunnel. Looks good.src/proto/tee-bundle.ts (1)
1-6: Generated header update is benignThe only modification is the
protocversion comment (v6.33.1). No functional changes in this file; as long as it was regenerated with that compiler, this is fine.src/client/create-claim.ts (1)
103-107: End-to-end wiring of proxySessionId through create/claim looks consistentReading
proxySessionIdviagetProviderValue(params, provider.proxySessionId, secretParams)and threading it intocreateTunnelReq(then reusing the same object inClaimTunnelRequest) keeps the tunnel creation and claim paths in sync. Combined with the TLS tunnel’s|| ''normalization, this is consistent with the proto’s default semantics.Also applies to: 130-137
src/server/handlers/claimTunnel.ts (1)
32-45: Including proxySessionId in tunnel request mismatch check is correctExtending the mismatch guard to compare
tunnel.createRequest?.proxySessionIdagainstrequest?.proxySessionIdaligns with the existing host/port/geo checks and prevents claims from mutating the proxy session ID post‑creation. Given both sides normalize absent values to"", this should behave correctly across versions.src/tests/http-provider-utils.test.ts (1)
21-31: proxySessionId tests and fixtures look correct and aligned with provider behaviorThe added coverage for
proxySessionId:
- Valid template resolution (
should get proxy sessionId),- Missing template param error,
- Treating
proxySessionId: ''asundefined,- And updated ProviderParams fixtures (including hash expectations)
all match the intended semantics of proxySessionId being optional and resolved via
paramValues.No changes needed here.
Also applies to: 88-90, 512-530, 962-995, 1105-1111, 1159-1164, 1281-1286
src/providers/http/index.ts (2)
57-61: LGTM!The
proxySessionIdmethod follows the same pattern asgeoLocationfor exposing provider parameters.
817-846: LGTM!The
getProxySessionIdfunction follows the same pattern asgetGeoLocation, correctly handling template parameter substitution and validation.src/tests/tcp-tunnel.test.ts (2)
19-34: LGTM!The
proxySessionId: ''addition correctly propagates the new parameter through tunnel creation tests.
58-86: LGTM!Good coverage for both invalid geoLocation and invalid proxySessionId validation, ensuring 400 errors are returned.
src/proto/api.ts (1)
1-5: Auto-generated file - changes verified correct.This file is generated by
protoc-gen-ts_protoand should not be manually edited. TheproxySessionIdfield (field 5, string type) is correctly defined in the sourceproto/api.protoand has been properly added toCreateTunnelRequestin the generated code with correct encoding, decoding, and JSON conversion paths.
predatorx7
force-pushed
the
same-proxy-ip
branch
from
f466ef8 to
841f400
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/tunnels/make-tcp-tunnel.ts (1)
216-228: Add conditional checks before validation to support optional parameters.After fixing line 211 to use
&&, eithergeoLocationorproxySessionIdmay be empty. The current validation logic will incorrectly reject empty strings:
isValidCountryCode('')likely returns falseisValidProxySessionId('')checkslength > 8, so empty string failsThis prevents the intended behavior of allowing proxy usage with only one parameter.
Apply this diff to validate only when values are provided:
- if(!isValidCountryCode(geoLocation)) { + if(geoLocation && !isValidCountryCode(geoLocation)) { throw AttestorError.badRequest( `Geolocation "${geoLocation}" is invalid. Must be 2 letter ISO country code`, { geoLocation } ) } - if(!isValidProxySessionId(proxySessionId)) { + if(proxySessionId && !isValidProxySessionId(proxySessionId)) { throw AttestorError.badRequest( `proxySessionId "${proxySessionId}" is invalid. Must be a lowercase alphanumeric string of length 8-14 characters. eg. "mystring12345", "something1234".`, { proxySessionId } ) }
♻️ Duplicate comments (6)
provider-schemas/http/parameters.yaml (1)
28-34: Grammar fix and consider adding schema constraints.The description has "an smallcase" which should be "a lowercase". To align with
isValidProxySessionIdvalidation, consider adding schema-level constraints:proxySessionId: type: string nullable: true minLength: 8 maxLength: 14 pattern: '^[a-z0-9]+$' description: >- Specify the unique session ID for allowing use of the same proxy IP across multiple requests. Must be a lowercase alphanumeric string of length 8-14 characters. e.g. "mystring12345", "something1234".src/server/utils/proxy-session.ts (1)
1-3: Length validation mismatch and style issues remain unaddressed.The length check
> 8 && < 15allows 9–14 characters, but documentation consistently states 8–14. Additionally, the function uses 4 spaces instead of tabs and has a trailing semicolon.proto/api.proto (1)
149-159: Contract mismatch with validator and grammar issue in documentation.The proto comment specifies 8–14 characters, but
isValidProxySessionIdenforces 9–14. Also, "an smallcase" should be "a lowercase".- * Can be an smallcase alphanumeric string of length 8-14 characters. + * Must be a lowercase alphanumeric string of length 8-14 characters.src/tests/tcp-tunnel.test.ts (1)
100-152: Good test coverage for proxySessionId IP persistence, but fix the assertion bug.The test logic and helper function are well-structured. However, line 151 has a critical bug:
a === b === cevaluates as(a === b) === c, comparing a boolean to a string.- assert.ok(resJson1.ip === resJson2.ip === resJson3.ip); + assert.strictEqual(resJson1.ip, resJson2.ip, 'IP should match across sessions'); + assert.strictEqual(resJson2.ip, resJson3.ip, 'IP should match across sessions');src/server/tunnels/make-tcp-tunnel.ts (2)
13-13: Use absolute import alias as previously suggested.This was flagged in a previous review. The import should use the project's absolute alias form instead of a relative path.
202-209: Include proxySessionId in warning context as previously suggested.This was flagged in a previous review. The logger.warn call should include both
geoLocationandproxySessionIdin its context object for better debugging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
proto/api.proto(1 hunks)provider-schemas/http/parameters.yaml(1 hunks)src/client/create-claim.ts(2 hunks)src/client/tunnels/make-rpc-tls-tunnel.ts(1 hunks)src/proto/api.ts(8 hunks)src/proto/tee-bundle.ts(1 hunks)src/providers/http/index.ts(4 hunks)src/server/handlers/claimTunnel.ts(1 hunks)src/server/handlers/createTunnel.ts(1 hunks)src/server/tunnels/make-tcp-tunnel.ts(7 hunks)src/server/utils/proxy-session.ts(1 hunks)src/tests/http-provider-utils.test.ts(6 hunks)src/tests/rpc-tunnel.test.ts(3 hunks)src/tests/tcp-tunnel.test.ts(3 hunks)src/types/providers.gen.ts(2 hunks)src/types/providers.ts(1 hunks)src/types/tunnel.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- src/client/create-claim.ts
- src/types/providers.gen.ts
- src/client/tunnels/make-rpc-tls-tunnel.ts
- src/server/handlers/createTunnel.ts
- src/server/handlers/claimTunnel.ts
- src/types/tunnel.ts
- src/proto/tee-bundle.ts
- src/tests/rpc-tunnel.test.ts
- src/providers/http/index.ts
- src/tests/http-provider-utils.test.ts
- src/types/providers.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/server/tunnels/make-tcp-tunnel.ts (4)
src/providers/http/index.ts (2)
proxySessionId(57-61)geoLocation(52-56)src/utils/logger.ts (1)
logger(10-10)src/server/utils/proxy-session.ts (1)
isValidProxySessionId(1-3)src/utils/error.ts (1)
AttestorError(10-66)
src/tests/tcp-tunnel.test.ts (5)
src/providers/http/index.ts (2)
geoLocation(52-56)proxySessionId(57-61)src/server/tunnels/make-tcp-tunnel.ts (1)
makeTcpTunnel(41-102)src/utils/error.ts (1)
AttestorError(10-66)src/utils/http-parser.ts (1)
makeHttpResponseParser(52-232)src/utils/generics.ts (2)
strToUint8Array(41-43)uint8ArrayToStr(34-36)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (6)
src/proto/api.ts (1)
1404-1404: Auto-generated protobuf code correctly adds proxySessionId support.The generated code properly handles the new
proxySessionIdfield across encode, decode, JSON conversion, and partial construction methods, consistent with the proto definition.Also applies to: 1421-1423, 1466-1473, 1489-1489, 1507-1509, 1522-1522
src/tests/tcp-tunnel.test.ts (1)
58-86: Test coverage for invalid proxySessionId is appropriate.The test correctly verifies that an invalid
proxySessionId(too short) triggers a 400 error. This aligns with the validation logic inisValidProxySessionId.src/server/tunnels/make-tcp-tunnel.ts (4)
31-32: LGTM!The documentation clearly explains the purpose of
proxySessionIdfor session-scoped proxy IP persistence.
104-104: LGTM!The
proxySessionIdparameter is correctly propagated through theconnectTcp→getSocket→_getSocketcall chain.Also applies to: 125-125, 197-197
230-236: LGTM!The URL construction correctly handles optional parameters, replacing placeholders with actual values or empty strings, and properly formatting the session suffix.
255-260: LGTM!Error logging and messages properly include
proxySessionIdcontext alongsidegeoLocation, which aids debugging and provides clear error information to users.
src/tests/tcp-tunnel.test.ts
Outdated
| assert.rejects( | ||
| async () => makeTcpTunnel({ | ||
| host: 'lumtest.com', | ||
| port: 80, | ||
| geoLocation: '', | ||
| proxySessionId: 'xz', | ||
| logger, | ||
| }), | ||
| (err: AttestorError) => { | ||
| assert.match(err.message, /failed with status code: 400/) | ||
| return true | ||
| } | ||
| ); |
There was a problem hiding this comment.
Await the rejection assertions.
Both assert.rejects calls are not awaited, so test failures may not be caught properly.
- assert.rejects(
+ await assert.rejects(
async () => makeTcpTunnel({
host: 'lumtest.com',
port: 80,
geoLocation: '',
proxySessionId: 'xz',
logger,
}),
(err: AttestorError) => {
assert.match(err.message, /failed with status code: 400/)
return true
}
);Also apply await to the first assert.rejects at line 59.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert.rejects( | |
| async () => makeTcpTunnel({ | |
| host: 'lumtest.com', | |
| port: 80, | |
| geoLocation: '', | |
| proxySessionId: 'xz', | |
| logger, | |
| }), | |
| (err: AttestorError) => { | |
| assert.match(err.message, /failed with status code: 400/) | |
| return true | |
| } | |
| ); | |
| await assert.rejects( | |
| async () => makeTcpTunnel({ | |
| host: 'lumtest.com', | |
| port: 80, | |
| geoLocation: '', | |
| proxySessionId: 'xz', | |
| logger, | |
| }), | |
| (err: AttestorError) => { | |
| assert.match(err.message, /failed with status code: 400/) | |
| return true | |
| } | |
| ); |
🤖 Prompt for AI Agents
In src/tests/tcp-tunnel.test.ts around lines 59 and 73-85, the two
assert.rejects calls are not awaited so test failures can be missed; add await
before both assert.rejects invocations (the one around line 59 and the one
spanning lines 73-85) so the test runner waits for the promise assertions to
complete and failures are properly propagated.
841f400 to
8a860a2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tests/rpc-tunnel.test.ts (1)
180-237: Remove extra semicolons flagged by lintStatic analysis reports extra semicolons on the two
assert.rejectsblocks’ closing lines. You can satisfy the linter by dropping those trailing;while keeping behavior the same:- }); + }) … - }); + })
♻️ Duplicate comments (7)
src/server/tunnels/make-tcp-tunnel.ts (3)
211-227: Fix proxy gating and make validations conditionalThe current condition:
if(!geoLocation || !proxySessionId) { socket.connect({ host, port }) return socket }forces BOTH geoLocation and proxySessionId to be present to use the proxy. That breaks intended behavior where either field alone should be enough to route via proxy. It also always validates both fields, even when one is intentionally omitted.
Recommend:
- if(!geoLocation || !proxySessionId) { + if(!geoLocation && !proxySessionId) { socket.connect({ host, port, }) return socket } - if(!isValidCountryCode(geoLocation)) { + if(geoLocation && !isValidCountryCode(geoLocation)) { throw AttestorError.badRequest( `Geolocation "${geoLocation}" is invalid. Must be 2 letter ISO country code`, { geoLocation } ) } - if(!isValidProxySessionId(proxySessionId)) { + if(proxySessionId && !isValidProxySessionId(proxySessionId)) { throw AttestorError.badRequest( `proxySessionId "${proxySessionId}" is invalid. Must be a lowercase alphanumeric string of length 8-14 characters. eg. "mystring12345", "something1234".`, { proxySessionId } ) }This keeps direct connections when both are absent, but allows proxy usage with geoLocation-only, proxySessionId-only, or both.
13-13: Use absolute import path for isValidProxySessionIdStatic analysis prefers the project’s absolute alias here. Update the import to be consistent with other imports in this file:
-import { isValidProxySessionId } from '../utils/proxy-session.ts' +import { isValidProxySessionId } from '#src/server/utils/proxy-session.ts'
201-209: Include proxySessionId in warning context when proxy URL is missingWhen a geoLocation or proxySessionId is supplied but
HTTPS_PROXY_URLis unset, the warn log currently only includes geoLocation. For debugging, it’s useful to see both:- if((proxySessionId || geoLocation) && !HTTPS_PROXY_URL) { - logger.warn( - { geoLocation }, - 'geoLocation or proxySessionId provided but no proxy URL found' - ) + if((proxySessionId || geoLocation) && !HTTPS_PROXY_URL) { + logger.warn( + { geoLocation, proxySessionId }, + 'geoLocation or proxySessionId provided but no proxy URL found' + ) geoLocation = '' proxySessionId = '' }src/tests/tcp-tunnel.test.ts (1)
58-86: Await rejection assertions and fix chained IP equalityTwo issues here:
assert.rejectscalls are not awaited, so failures may not be caught if this suite is ever un‑skipped.resJson1.ip === resJson2.ip === resJson3.ipis a chained equality that compares a boolean to a string and doesn’t actually assert that all three IPs match.Suggested fixes:
- it('should gracefully fail an invalid geoLocation or ip session id', async () => { - assert.rejects( + it('should gracefully fail an invalid geoLocation or ip session id', async () => { + await assert.rejects( async () => makeTcpTunnel({ host: 'lumtest.com', port: 80, geoLocation: 'xz', proxySessionId: '', logger, }), (err: AttestorError) => { assert.match(err.message, /failed with status code: 400/) return true } - ); - - assert.rejects( + ) + + await assert.rejects( async () => makeTcpTunnel({ host: 'lumtest.com', port: 80, geoLocation: '', proxySessionId: 'xz', logger, }), (err: AttestorError) => { assert.match(err.message, /failed with status code: 400/) return true } - ); + ) }) … - assert.ok(resJson1.ip === resJson2.ip === resJson3.ip); + assert.strictEqual(resJson1.ip, resJson2.ip) + assert.strictEqual(resJson2.ip, resJson3.ip)Also applies to: 100-152
src/server/utils/proxy-session.ts (1)
1-3: Fix proxySessionId length range and lint issuesCurrent validator enforces 9–14 chars (
> 8 && < 15), but the rest of the code/docs say 8–14. It also violates the project’s no‑semicolon / tabs style.Recommend:
-export function isValidProxySessionId(sessionId: string): boolean { - return typeof sessionId === 'string' && sessionId.length > 8 && sessionId.length < 15 && /^[a-z0-9]+$/.test(sessionId); -} +export function isValidProxySessionId(sessionId: string): boolean { + return typeof sessionId === 'string' + && sessionId.length >= 8 + && sessionId.length <= 14 + && /^[a-z0-9]+$/.test(sessionId) +}This matches the documented 8–14 range and satisfies the linter.
src/providers/http/index.ts (2)
33-33: Import order issue persists despite previous fix.The pipeline is still failing due to import sorting issues. The previous fix may not have fully resolved the problem.
Run ESLint autofix to correct the import order:
#!/bin/bash # Run ESLint autofix to sort imports npx eslint --fix src/providers/http/index.ts
698-702: Critical bug: Wrong property namegetProxySessionIdshould beproxySessionId.This code uses
params.getProxySessionIdbut thegetProxySessionIdfunction at line 818 expectsv2Params.proxySessionId. Compare with thegeoLocationpattern at lines 693-697 which correctly usesparams.geoLocation.Additionally, add a blank line before this statement per linting rules.
Apply this diff to fix the property name and linting issue:
+ -const proxySessionIdParams = extractAndReplaceTemplateValues(params.getProxySessionId) +const proxySessionIdParams = extractAndReplaceTemplateValues(params.proxySessionId) if(proxySessionIdParams) { - params.getProxySessionId = proxySessionIdParams.newParam + params.proxySessionId = proxySessionIdParams.newParam extractedValues = { ...extractedValues, ...proxySessionIdParams.extractedValues } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
proto/api.proto(1 hunks)provider-schemas/http/parameters.yaml(1 hunks)src/client/create-claim.ts(2 hunks)src/client/tunnels/make-rpc-tls-tunnel.ts(1 hunks)src/proto/api.ts(8 hunks)src/proto/tee-bundle.ts(1 hunks)src/providers/http/index.ts(4 hunks)src/server/handlers/claimTunnel.ts(1 hunks)src/server/handlers/createTunnel.ts(1 hunks)src/server/tunnels/make-tcp-tunnel.ts(7 hunks)src/server/utils/proxy-session.ts(1 hunks)src/tests/http-provider-utils.test.ts(6 hunks)src/tests/rpc-tunnel.test.ts(3 hunks)src/tests/tcp-tunnel.test.ts(3 hunks)src/types/providers.gen.ts(2 hunks)src/types/providers.ts(1 hunks)src/types/tunnel.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/proto/tee-bundle.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- src/types/providers.ts
- src/client/create-claim.ts
- src/server/handlers/createTunnel.ts
- src/client/tunnels/make-rpc-tls-tunnel.ts
- provider-schemas/http/parameters.yaml
- src/types/tunnel.ts
- proto/api.proto
- src/proto/api.ts
- src/types/providers.gen.ts
🧰 Additional context used
🧬 Code graph analysis (4)
src/tests/rpc-tunnel.test.ts (2)
src/client/tunnels/make-rpc-tls-tunnel.ts (1)
makeRpcTlsTunnel(32-172)src/utils/error.ts (1)
AttestorError(10-66)
src/server/tunnels/make-tcp-tunnel.ts (2)
src/server/utils/proxy-session.ts (1)
isValidProxySessionId(1-3)src/utils/error.ts (1)
AttestorError(10-66)
src/tests/tcp-tunnel.test.ts (4)
src/providers/http/index.ts (2)
geoLocation(52-56)proxySessionId(57-61)src/server/tunnels/make-tcp-tunnel.ts (1)
makeTcpTunnel(41-102)src/utils/http-parser.ts (1)
makeHttpResponseParser(52-232)src/utils/generics.ts (2)
strToUint8Array(41-43)uint8ArrayToStr(34-36)
src/providers/http/index.ts (2)
src/types/providers.ts (1)
ProviderSecretParams(29-29)src/server/utils/proxy-session.ts (1)
isValidProxySessionId(1-3)
🪛 GitHub Actions: Test
src/providers/http/index.ts
[error] 1-1: eslint ./ --ext .js,.ts,.jsx,.tsx: simple-import-sort/imports - Run autofix to sort these imports!
🪛 GitHub Check: test
src/tests/rpc-tunnel.test.ts
[failure] 237-237:
Extra semicolon
[failure] 208-208:
Extra semicolon
src/server/tunnels/make-tcp-tunnel.ts
[failure] 13-13:
import statements should have an absolute path
[failure] 236-236:
Extra semicolon
[failure] 230-230:
This assertion is unnecessary since it does not change the type of the expression
src/providers/http/index.ts
[failure] 698-698:
Expected blank line before this statement
src/server/utils/proxy-session.ts
[failure] 2-2:
Extra semicolon
[failure] 2-2:
Expected indentation of 1 tab but found 4 spaces
🔇 Additional comments (6)
src/server/handlers/claimTunnel.ts (1)
38-45: Include proxySessionId in create/claim consistency checksAdding
proxySessionIdto the mismatch guard is correct and keeps claim validation aligned with the extendedCreateTunnelRequestsurface.src/tests/rpc-tunnel.test.ts (1)
36-38: Explicitly passing empty geoLocation/proxySessionId looks goodInitializing
geoLocationandproxySessionIdas empty strings in the basic RPC tunnel test clarifies intent and keeps payloads consistent with the server defaults.src/tests/http-provider-utils.test.ts (1)
23-30: proxySessionId test coverage and plumbing look consistentThe added
proxySessionIdhandling in HTTP provider tests is consistent with existinggeoLocationpatterns:
getProviderValue+proxySessionIdtemplate resolution.- Error path when the template param is missing.
- Empty
proxySessionIdreturningundefined.- Including
proxySessionIdinhashProviderParamsandcreateRequestflows.All of this aligns with the new provider surface and should help catch regressions.
Also applies to: 512-515, 962-994, 1045-1111, 1159-1183, 1281-1286
src/server/tunnels/make-tcp-tunnel.ts (1)
252-261: Proxy error message composition is correct and informativeThe updated error message that conditionally mentions geoLocation and/or proxySessionId produces clear diagnostics while handling the “only one provided” cases cleanly. This is a nice improvement for debugging proxy failures.
src/providers/http/index.ts (2)
57-61: LGTM!The
proxySessionIdmethod correctly follows the same pattern asgeoLocation, delegating togetProxySessionIdwhen the parameter is present.
817-846: LGTM!The
getProxySessionIdfunction correctly implements template parameter resolution and validation, following the same pattern asgetGeoLocation. The validation withisValidProxySessionIdensures only valid proxy session IDs are accepted.
predatorx7
force-pushed
the
same-proxy-ip
branch
from
8a860a2 to
f4cce1d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tests/rpc-tunnel.test.ts (1)
180-209: Remove extra semicolon.Static analysis flags an extra semicolon that violates the project's style guidelines.
Apply this diff:
(err: AttestorError) => { assert.match(err.message, /Geolocation "XZ" is invalid/) return true } - ); + )
♻️ Duplicate comments (11)
proto/api.proto (1)
149-159: Contract mismatch remains unresolved between proto docs and validator.The proto documentation states proxySessionId can be 8-14 characters, but based on past review comments, there's an inconsistency with the server-side validator. The team must decide on the canonical allowed length (either 8-14 or 9-14) and ensure:
- Proto documentation
- Server validation logic (isValidProxySessionId)
- Type definitions
- Tests
all agree on the same character range.
src/server/utils/proxy-session.ts (1)
1-3: Fix formatting issues: indentation and semicolon.Static analysis flags incorrect indentation (4 spaces instead of 1 tab) and an extra semicolon. The project uses tab indentation.
Apply this diff:
export function isValidProxySessionId(sessionId: string): boolean { - return typeof sessionId === 'string' && sessionId.length >= 8 && sessionId.length < 15 && /^[a-z0-9]+$/.test(sessionId); + return typeof sessionId === 'string' + && sessionId.length >= 8 + && sessionId.length <= 14 + && /^[a-z0-9]+$/.test(sessionId) }Note: Also changed
< 15to<= 14for clarity, though both are semantically equivalent.src/tests/tcp-tunnel.test.ts (4)
12-12: Fix formatting: remove space afterfor.Static analysis flags unexpected whitespace after the
forkeyword, which violates the project's style guidelines.Apply this diff:
- for (const geoLocation of ALL_LOCATIONS) { + for(const geoLocation of ALL_LOCATIONS) {
59-71: Missingawaiton assertion.The
assert.rejectscall is not awaited, which means test failures may not be properly caught and the test may complete before the assertion is evaluated.Apply this diff:
- assert.rejects( + await assert.rejects( async () => makeTcpTunnel({
73-85: Missingawaiton assertion.The
assert.rejectscall is not awaited, which means test failures may not be properly caught and the test may complete before the assertion is evaluated.Apply this diff:
- assert.rejects( + await assert.rejects( async () => makeTcpTunnel({
151-151: Fix chained equality comparison bug.In JavaScript,
a === b === cevaluates as(a === b) === c, comparing a boolean to a string. This will never correctly verify that all three IPs are equal.Apply this diff:
- assert.ok(resJson1.ip === resJson2.ip === resJson3.ip); + assert.strictEqual(resJson1.ip, resJson2.ip, 'IP should match between requests') + assert.strictEqual(resJson2.ip, resJson3.ip, 'IP should match between requests')src/server/tunnels/make-tcp-tunnel.ts (3)
13-13: Use project alias import for isValidProxySessionIdThe import should follow the project’s absolute alias convention (and the lint hint) instead of a relative path.
-import { isValidProxySessionId } from '../utils/proxy-session.ts' +import { isValidProxySessionId } from '#src/server/utils/proxy-session.ts'This will also clear the “absolute path” ESLint error.
192-214: Proxy routing condition forces both geoLocation and proxySessionId; should allow either
_getSocketcurrently skips the proxy whenever eithergeoLocationorproxySessionIdis missing:if(!geoLocation || !proxySessionId) { socket.connect({ host, port }) return socket }This requires both to be present to use the proxy, which contradicts the surrounding logic and intended behavior (proxy should be used when at least one of them is provided).
Update the condition so that you only bypass the proxy when both are absent:
- if(!geoLocation || !proxySessionId) { + if(!geoLocation && !proxySessionId) { socket.connect({ host, port }) return socket }
201-209: Tighten logging and simplify agent URL constructionTwo small cleanups here:
- Include proxySessionId in the warning context
You already clear
proxySessionIdwhen no proxy URL is configured; include it in the log for debugging:- if((proxySessionId || geoLocation) && !HTTPS_PROXY_URL) { - logger.warn( - { geoLocation }, - 'geoLocation or proxySessionId provided but no proxy URL found' - ) + if((proxySessionId || geoLocation) && !HTTPS_PROXY_URL) { + logger.warn( + { geoLocation, proxySessionId }, + 'geoLocation or proxySessionId provided but no proxy URL found' + )
- Remove unnecessary non-null assertions and trailing semicolon on agentUrl
HTTPS_PROXY_URLalready has a concrete type fromgetEnvVariable, so the!assertions and the extra;are not needed and are flagged by lint:- const agentUrl = HTTPS_PROXY_URL!.replace( + const agentUrl = HTTPS_PROXY_URL.replace( '{{geoLocation}}', geoLocation?.toLowerCase() || '' - )!.replace( + ).replace( '{{proxySessionId}}', proxySessionId ? `-session-${proxySessionId}` : '' - ); + )The
isValidProxySessionIdcheck and the badRequest error look correct and give a clear message.Also applies to: 223-227, 230-236
src/providers/http/index.ts (2)
33-33: Fix import ordering per simple-import-sortThe new
isValidProxySessionIdimport triggers the simple-import-sort ESLint rule. Reorder imports to match the project’s configured grouping (third-party,#src/..., then relative), or just run:npx eslint --fix src/providers/http/index.tsto let the tool sort them.
693-702: Bug: substituteParamValues uses wrong property for proxySessionId templatesHere you attempt to resolve template values for
proxySessionId, but the code reads and writesparams.getProxySessionId, which doesn’t exist onHTTPProviderParams:const proxySessionIdParams = extractAndReplaceTemplateValues(params.getProxySessionId) if(proxySessionIdParams) { params.getProxySessionId = proxySessionIdParams.newParam extractedValues = { ...extractedValues, ...proxySessionIdParams.extractedValues } }This means proxySessionId templates are never actually substituted on
params.proxySessionId.Update this block to operate on the correct property:
- const proxySessionIdParams = extractAndReplaceTemplateValues(params.getProxySessionId) + const proxySessionIdParams = extractAndReplaceTemplateValues(params.proxySessionId) if(proxySessionIdParams) { - params.getProxySessionId = proxySessionIdParams.newParam + params.proxySessionId = proxySessionIdParams.newParam extractedValues = { ...extractedValues, ...proxySessionIdParams.extractedValues } }Also, add the blank line before this
constto satisfy the ESLint “expected blank line before this statement” rule.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
proto/api.proto(1 hunks)provider-schemas/http/parameters.yaml(1 hunks)src/client/create-claim.ts(2 hunks)src/client/tunnels/make-rpc-tls-tunnel.ts(1 hunks)src/proto/api.ts(8 hunks)src/proto/tee-bundle.ts(1 hunks)src/providers/http/index.ts(4 hunks)src/server/handlers/claimTunnel.ts(1 hunks)src/server/handlers/createTunnel.ts(1 hunks)src/server/tunnels/make-tcp-tunnel.ts(7 hunks)src/server/utils/proxy-session.ts(1 hunks)src/tests/http-provider-utils.test.ts(6 hunks)src/tests/rpc-tunnel.test.ts(3 hunks)src/tests/tcp-tunnel.test.ts(3 hunks)src/types/providers.gen.ts(2 hunks)src/types/providers.ts(1 hunks)src/types/tunnel.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- provider-schemas/http/parameters.yaml
- src/client/tunnels/make-rpc-tls-tunnel.ts
- src/server/handlers/claimTunnel.ts
- src/server/handlers/createTunnel.ts
- src/client/create-claim.ts
- src/proto/tee-bundle.ts
- src/types/providers.ts
- src/types/providers.gen.ts
🧰 Additional context used
🧬 Code graph analysis (6)
src/tests/rpc-tunnel.test.ts (2)
src/client/tunnels/make-rpc-tls-tunnel.ts (1)
makeRpcTlsTunnel(32-172)src/utils/error.ts (1)
AttestorError(10-66)
src/providers/http/index.ts (2)
src/types/providers.ts (1)
ProviderSecretParams(29-29)src/server/utils/proxy-session.ts (1)
isValidProxySessionId(1-3)
src/server/tunnels/make-tcp-tunnel.ts (3)
src/providers/http/index.ts (2)
proxySessionId(57-61)geoLocation(52-56)src/server/utils/proxy-session.ts (1)
isValidProxySessionId(1-3)src/utils/error.ts (1)
AttestorError(10-66)
src/tests/tcp-tunnel.test.ts (4)
src/providers/http/index.ts (2)
geoLocation(52-56)proxySessionId(57-61)src/server/tunnels/make-tcp-tunnel.ts (1)
makeTcpTunnel(41-102)src/utils/http-parser.ts (1)
makeHttpResponseParser(52-232)src/utils/generics.ts (2)
strToUint8Array(41-43)uint8ArrayToStr(34-36)
src/tests/http-provider-utils.test.ts (3)
src/utils/generics.ts (1)
getProviderValue(145-150)src/types/providers.ts (1)
ProviderParams(27-27)src/providers/http/index.ts (1)
proxySessionId(57-61)
src/types/tunnel.ts (1)
src/proto/api.ts (2)
CreateTunnelRequest(423-452)CreateTunnelRequest(1407-1525)
🪛 GitHub Actions: Test
src/providers/http/index.ts
[error] 1-1: ESLint: simple-import-sort/imports - Run autofix to sort these imports.
🪛 GitHub Check: test
src/tests/rpc-tunnel.test.ts
[failure] 237-237:
Extra semicolon
[failure] 208-208:
Extra semicolon
src/providers/http/index.ts
[failure] 698-698:
Expected blank line before this statement
src/server/tunnels/make-tcp-tunnel.ts
[failure] 13-13:
import statements should have an absolute path
[failure] 236-236:
Extra semicolon
[failure] 230-230:
This assertion is unnecessary since it does not change the type of the expression
src/server/utils/proxy-session.ts
[failure] 2-2:
Extra semicolon
[failure] 2-2:
Expected indentation of 1 tab but found 4 spaces
🔇 Additional comments (8)
src/proto/api.ts (1)
4-4: LGTM! Generated code correctly implements proxySessionId.The protoc-generated code properly handles the new proxySessionId field across all serialization paths (encode, decode, fromJSON, toJSON, fromPartial). The field is correctly assigned field number 5 with wire type 42 (string), and the initialization defaults to an empty string.
Also applies to: 441-451, 1404-1404, 1421-1423, 1466-1473, 1489-1489, 1507-1509, 1522-1522
src/types/tunnel.ts (1)
24-24: LGTM! Type signature correctly extended.The TCPSocketProperties type now includes proxySessionId in the createRequest pick, properly reflecting the updated CreateTunnelRequest interface.
src/tests/rpc-tunnel.test.ts (1)
36-38: LGTM! Test properly initialized with proxySessionId.The tunnel creation request now correctly includes the proxySessionId field with an empty string default, consistent with the API changes.
src/tests/tcp-tunnel.test.ts (1)
100-152: Good test coverage for proxySessionId IP persistence.The new test properly verifies that multiple requests using the same proxySessionId yield consistent IP addresses, which is the core feature being implemented.
src/tests/http-provider-utils.test.ts (2)
23-30: Proxy session ID wiring in tests looks consistent with provider behaviorDestructuring
proxySessionIdfrom the HTTP provider and threading it through hash, body, and URL substitution tests (including explicit''cases) correctly exercises the new parameter without changing existing semantics. No issues here.Also applies to: 494-530, 1104-1111, 1159-1164, 1281-1286
962-995: Good coverage for proxySessionId resolution and edge casesThe new tests validate:
- Template-based resolution via
getProviderValueandparamValues- Error on missing template param
undefinedwhenproxySessionIdis emptyThis matches the HTTP provider’s
getProxySessionIdlogic and will catch regressions. No changes needed.src/server/tunnels/make-tcp-tunnel.ts (1)
223-227: Proxy session ID validation aligns with shared rulesThe
isValidProxySessionIdcheck and the specificAttestorError.badRequestmessage (including echoing the bad value and documenting constraints) correctly enforce the shared validation rule and provide structured error data for clients.src/providers/http/index.ts (1)
57-61: HTTP provider proxySessionId wiring and validation look correctDefining
proxySessionId(params, secretParams)onHTTP_PROVIDERand delegating togetProxySessionIdmirrors the existinggeoLocationpattern, andgetProxySessionId:
- Resolves
{{...}}placeholders fromparamValuesand secretparamValues- Throws with a clear message when a template param is missing
- Validates format via
isValidProxySessionIdand errors on invalid values- Returns
undefinedwhenproxySessionIdis unset or emptyThis matches the new tests and shared validation helper; no functional issues here.
Also applies to: 785-846
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/tunnels/make-tcp-tunnel.ts (1)
201-228: Fix validation so geo-only and proxySessionId-only usages both workRight now
_getSocketalways validates bothgeoLocationandproxySessionIdwhenever at least one is non-empty:
- If
geoLocation = 'US'andproxySessionId = '', the call toisValidProxySessionId('')will fail, breaking existing geo-only behavior.- If
geoLocation = ''andproxySessionId = 'abcd12345', the call toisValidCountryCode('')will fail, blocking the intended “session-only” proxy use-case (as used in the new TCP tests).The guard above (
if(!geoLocation && !proxySessionId) { ... }) correctly routes the “both absent” case directly, so these validations should be conditional on the corresponding field actually being provided.Suggested fix:
- if(!isValidCountryCode(geoLocation)) { - throw AttestorError.badRequest( - `Geolocation "${geoLocation}" is invalid. Must be 2 letter ISO country code`, - { geoLocation } - ) - } - - if(!isValidProxySessionId(proxySessionId)) { - throw AttestorError.badRequest( - `proxySessionId "${proxySessionId}" is invalid. Must be a lowercase alphanumeric string of length 8-14 characters. eg. "mystring12345", "something1234".`, - { proxySessionId } - ) - } + if(geoLocation && !isValidCountryCode(geoLocation)) { + throw AttestorError.badRequest( + `Geolocation "${geoLocation}" is invalid. Must be 2 letter ISO country code`, + { geoLocation } + ) + } + + if(proxySessionId && !isValidProxySessionId(proxySessionId)) { + throw AttestorError.badRequest( + `proxySessionId "${proxySessionId}" is invalid. Must be a lowercase alphanumeric string of length 8-14 characters. eg. "mystring12345", "something1234".`, + { proxySessionId } + ) + }
♻️ Duplicate comments (3)
src/tests/tcp-tunnel.test.ts (1)
58-85: await theassert.rejectscalls so failures are actually assertedBoth rejection assertions are still not awaited, so the async test may complete before the assertions run, and failures can be missed.
Apply this change:
- it('should gracefully fail an invalid geoLocation or ip session id', async() => { - assert.rejects( + it('should gracefully fail an invalid geoLocation or ip session id', async() => { + await assert.rejects( async() => makeTcpTunnel({ host: 'lumtest.com', port: 80, geoLocation: 'xz', proxySessionId: '', logger, }), (err: AttestorError) => { assert.match(err.message, /failed with status code: 400/) return true } ) - assert.rejects( + await assert.rejects( async() => makeTcpTunnel({ host: 'lumtest.com', port: 80, geoLocation: '', proxySessionId: 'xz', logger, }), (err: AttestorError) => { assert.match(err.message, /failed with status code: 400/) return true } )src/server/tunnels/make-tcp-tunnel.ts (1)
202-236: Minor: non-null assertion on HTTPS_PROXY_URL is unnecessaryGiven
HTTPS_PROXY_URLis already a concrete string (fromgetEnvVariable) and guarded above when missing, the!non-null assertion is redundant and was previously flagged by static analysis.You can simplify:
- const agentUrl = HTTPS_PROXY_URL!.replace( + const agentUrl = HTTPS_PROXY_URL.replace( '{{geoLocation}}', geoLocation?.toLowerCase() || '' - ).replace( + ).replace( '{{proxySessionId}}', proxySessionId ? `-session-${proxySessionId}` : '' )src/providers/http/index.ts (1)
699-703: Bug:params.getProxySessionIdshould beparams.proxySessionIdThis block operates on
params.getProxySessionId, but the rest of the file (and the newgetProxySessionIdhelper) consistently useproxySessionIdas the parameter name. As previously noted, this will at best be a no-op and at worst a type/lint error, and it means template substitution for proxySessionId never actually runs.Suggested fix:
- const proxySessionIdParams = extractAndReplaceTemplateValues(params.getProxySessionId) - if(proxySessionIdParams) { - params.getProxySessionId = proxySessionIdParams.newParam - extractedValues = { ...extractedValues, ...proxySessionIdParams.extractedValues } - } + const proxySessionIdParams = extractAndReplaceTemplateValues(params.proxySessionId) + if(proxySessionIdParams) { + params.proxySessionId = proxySessionIdParams.newParam + extractedValues = { ...extractedValues, ...proxySessionIdParams.extractedValues } + }
🧹 Nitpick comments (1)
src/tests/tcp-tunnel.test.ts (1)
100-153: External IP check is useful but fragile; consider isolating itThe new “same IP using a proxySessionId” test is a good behavioral check for sticky proxy sessions, but it depends on
api.ipify.organd real network access, which can make it flaky or slow if this suite is ever un-skipped. Consider either keeping this permanently skipped or moving it to an integration/e2e suite with appropriate guards or mocking.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/providers/http/index.ts(4 hunks)src/server/tunnels/make-tcp-tunnel.ts(7 hunks)src/server/utils/proxy-session.ts(1 hunks)src/tests/rpc-tunnel.test.ts(3 hunks)src/tests/tcp-tunnel.test.ts(3 hunks)src/types/providers.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/server/utils/proxy-session.ts
- src/types/providers.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/providers/http/index.ts (2)
src/types/providers.ts (1)
ProviderSecretParams(29-29)src/server/utils/proxy-session.ts (1)
isValidProxySessionId(1-6)
src/server/tunnels/make-tcp-tunnel.ts (3)
src/providers/http/index.ts (2)
proxySessionId(57-61)geoLocation(52-56)src/server/utils/proxy-session.ts (1)
isValidProxySessionId(1-6)src/utils/error.ts (1)
AttestorError(10-66)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (7)
src/tests/rpc-tunnel.test.ts (2)
33-38: Explicitly passing empty geoLocation/proxySessionId looks fineAdding
geoLocation: ''andproxySessionId: ''to the RPC createTunnel payload keeps the new fields explicit without changing behavior (they’re effectively “unset” values). No issues here.
180-237: Good split of validation coverage between geoLocation and proxySessionIdThe two
assert.rejectsblocks clearly distinguish invalidgeoLocationfrom invalidproxySessionIdand correctly await the promises, so TLS tunnel creation failures are now well-covered.src/tests/tcp-tunnel.test.ts (2)
19-25: Adding proxySessionId to existing geoLocation tests is consistentPassing
proxySessionId: ''alongside existinggeoLocationvalues keeps tests aligned with the new API surface and continues to exercise pure geo-based routing. Once server-side validation treats empty proxy session IDs as “absent”, these tests will remain valid.
88-95: Explicit proxySessionId in restricted-server test is fineIncluding
proxySessionId: ''here keeps this test aligned with the new tunnel API while preserving the original behavior (no sticky proxy session). No further changes needed.src/server/tunnels/make-tcp-tunnel.ts (1)
202-209: Good: proxy warning now includes proxySessionId contextIncluding both
geoLocationandproxySessionIdin the warning whenHTTPS_PROXY_URLis missing will make debugging misconfigured environments much easier, and clearing both fields before falling back to direct connect is a sensible default.src/providers/http/index.ts (2)
57-61: HTTP provider proxySessionId entry point looks correctExposing
proxySessionId(params, secretParams)and delegating togetProxySessionIdmirrors the existinggeoLocationpattern and cleanly plugs into the Provider API. No issues here.
818-847: Proxy session ID resolution and validation are reasonable
getProxySessionIdcorrectly:
- Resolves
{{param}}placeholders fromparamValues/ secretparamValues.- Throws if any referenced template value is missing.
- Validates the final string via
isValidProxySessionIdbefore returning it.This matches the intended constraints from
isValidProxySessionIdand aligns withgeoLocationhandling.
predatorx7
force-pushed
the
same-proxy-ip
branch
from
d283227 to
cd39003
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/tunnels/make-tcp-tunnel.ts (1)
192-261: Bug: geoLocation validation breaks “proxySessionId-only” usageRight now, proxy usage requires a valid
geoLocationeven when onlyproxySessionIdis provided:
- The guard
if(!geoLocation && !proxySessionId)correctly allows proxy when either is present.- But immediately after,
if(!isValidCountryCode(geoLocation))runs unconditionally; whengeoLocationis empty/undefined andproxySessionIdis set, this throws a bad request instead of using the proxy with session-based routing.This effectively prevents using the proxy with only
proxySessionId, which contradicts the intended “either geoLocation or proxySessionId (or both)” behavior.You can fix this by only validating geoLocation when it’s provided:
- if(!isValidCountryCode(geoLocation)) { - throw AttestorError.badRequest( - `Geolocation "${geoLocation}" is invalid. Must be 2 letter ISO country code`, - { geoLocation } - ) - } + if(geoLocation && !isValidCountryCode(geoLocation)) { + throw AttestorError.badRequest( + `Geolocation "${geoLocation}" is invalid. Must be 2 letter ISO country code`, + { geoLocation } + ) + }This keeps strict validation when geoLocation is supplied, while allowing session-only proxying with a validated
proxySessionId.
♻️ Duplicate comments (1)
src/providers/http/index.ts (1)
693-703: Bug (regression): usingparams.getProxySessionIdinstead ofparams.proxySessionIdThis block is still operating on a non-existent
getProxySessionIdfield:const proxySessionIdParams = extractAndReplaceTemplateValues(params.getProxySessionId) if(proxySessionIdParams) { params.getProxySessionId = proxySessionIdParams.newParam // ... }Issues:
HTTPProviderParamsexposesproxySessionId, notgetProxySessionId, so this is a type error and will never actually update the realproxySessionIdvalue.- It also contradicts the usage in
getProxySessionId(...), which correctly readsv2Params.proxySessionId.This was already called out in a previous review; it still needs to be fixed.
Apply this diff to wire template substitution to the correct field:
- const proxySessionIdParams = extractAndReplaceTemplateValues(params.getProxySessionId) + const proxySessionIdParams = extractAndReplaceTemplateValues(params.proxySessionId) if(proxySessionIdParams) { - params.getProxySessionId = proxySessionIdParams.newParam + params.proxySessionId = proxySessionIdParams.newParam extractedValues = { ...extractedValues, ...proxySessionIdParams.extractedValues } }
🧹 Nitpick comments (1)
src/providers/http/index.ts (1)
818-847: getProxySessionId implementation is correct; consider minor reuse with getGeoLocationThe new
getProxySessionId:
- Resolves
{{param}}placeholders againstv2Params.paramValuesandsecretParams.paramValues, with clear erroring on missing values.- Validates the final string via
isValidProxySessionIdbefore returning.- Mirrors the existing
getGeoLocationpattern, so behavior is predictable for callers.If you want to reduce duplication, you could later factor the shared “template resolution” loop used here and in
getGeoLocationinto a small helper, but that’s non-blocking.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/providers/http/index.ts(4 hunks)src/server/tunnels/make-tcp-tunnel.ts(7 hunks)src/server/utils/proxy-session.ts(1 hunks)src/tests/rpc-tunnel.test.ts(3 hunks)src/tests/tcp-tunnel.test.ts(3 hunks)src/types/providers.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/server/utils/proxy-session.ts
- src/types/providers.ts
- src/tests/tcp-tunnel.test.ts
- src/tests/rpc-tunnel.test.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/server/tunnels/make-tcp-tunnel.ts (3)
src/utils/logger.ts (1)
logger(10-10)src/server/utils/proxy-session.ts (1)
isValidProxySessionId(1-6)src/utils/error.ts (1)
AttestorError(10-66)
src/providers/http/index.ts (2)
src/types/providers.ts (1)
ProviderSecretParams(29-29)src/server/utils/proxy-session.ts (1)
isValidProxySessionId(1-6)
🔇 Additional comments (3)
src/server/tunnels/make-tcp-tunnel.ts (2)
9-32: Import and documentation updates for proxySessionId look consistentUsing the absolute import
#src/server/utils/proxy-session.tsand extending the doc comment to describe proxySessionId’s role in keeping a static proxy IP are both aligned with the rest of the module and the new feature intent; no issues here.
104-152: Plumbing proxySessionId through connectTcp/getSocket is correctPassing proxySessionId from
connectTcpintogetSocket(and thus_getSocket) mirrors the existing geoLocation flow, keeps logging intact, and respects the existing timeout/error pattern; the control flow here looks sound.src/providers/http/index.ts (1)
17-61: HTTP_PROVIDER.proxySessionId surface and validation hook look goodThe new
proxySessionId(params, secretParams)accessor cleanly mirrorsgeoLocation, delegating togetProxySessionIdand returningundefinedwhen the field is absent. Combined with the dedicatedisValidProxySessionIdimport, this gives a clear, typed entry point for session-based proxy usage.
predatorx7
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/providers/http/index.ts (1)
818-847:getProxySessionIdlogic is sound; consider small refinements/DRY later.Implementation correctly:
- Resolves
{{param}}placeholders fromparamValuesorsecretParams.paramValues, and- Validates the final value via
isValidProxySessionId, throwing on missing or invalid values.Two optional follow‑ups you might consider (not blocking this PR):
- Treat empty strings explicitly (either normalizing to
undefinedor treating them as invalid) rather than relying on the truthiness check inif(v2Params?.proxySessionId).- Factor out the shared “resolve template with paramValues + secretParams” pattern used here, in
getGeoLocation, and ingetURLinto a small helper to reduce duplication and keep future changes to templating behavior in one place.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/providers/http/index.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/providers/http/index.ts (2)
src/types/providers.ts (1)
ProviderSecretParams(29-29)src/server/utils/proxy-session.ts (1)
isValidProxySessionId(1-6)
🔇 Additional comments (3)
src/providers/http/index.ts (3)
17-17: Import ofisValidProxySessionIdcorrectly wires centralized validation.The import path and usage align with the shared proxy-session validator; no issues here.
57-61:HTTP_PROVIDER.proxySessionIdhook is consistent with existing provider API.The new method mirrors the
geoLocationpattern and cleanly delegates togetProxySessionId, returningundefinedwhen the parameter is not present. Looks correct and type-safe.
699-703: Template substitution forproxySessionIdis correctly integrated.Handling
proxySessionIdinsubstituteParamValuesmatches the existing patterns (URL, body, geoLocation): templates are resolved, the field is updated in-place, andextractedValuesis merged. This should play well with downstream extraction/redaction logic.
…ssionId using a geolocation
2 participants
… proxy ip
Description
Testing (ignore for documentation update)
Type of change
Checklist:
Additional Notes:
Summary by CodeRabbit
New Features
Bug Fixes / Validation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.