Update Lockr integration to use dedicated trust-server SDK#395
Update Lockr integration to use dedicated trust-server SDK#395ChristianPavilonis wants to merge 3 commits intomainfrom
Conversation
Lockr now provides a pre-configured SDK (identity-lockr-trust-server.js) that routes API calls through the first-party proxy out of the box. This eliminates the need for fragile runtime regex rewriting of obfuscated host assignments in the SDK JavaScript. - Update default sdk_url to identity-lockr-trust-server.js - Remove rewrite_sdk_host config field, regex method, and related tests - Simplify handle_sdk_serving to serve SDK as-is (no host rewriting) - Fix operator precedence in is_lockr_sdk_url (clarity, no behavioral change) - Add debug logging to attribute rewriter for diagnosing rewrite issues - Extract test helpers, add trust-server URL detection test
92cc71c to
c2f1452
Compare
aram356
left a comment
There was a problem hiding this comment.
Summary
Good simplification — replaces fragile regex-based SDK host rewriting with a dedicated trust-server SDK from Lockr. Removes the regex dependency from this module, cleans up tests, and reduces code by ~190 lines. Two issues need attention before merge: the aim.loc.kr URL matching is overly broad, and removing rewrite_sdk_host without a deprecation warning risks silent privacy regressions for existing deployments.
Blocking
🔧 wrench
- Overly broad
aim.loc.krURL matching: any URL on that domain matches, including non-JS resources (lockr.rs:94) - Silent config drop for
rewrite_sdk_host: existing deployments lose host rewriting without warning (lockr.rs:31)
Non-blocking
🤔 thinking
- Contract coverage regressed: no test asserts the new SDK-served-as-is contract (
lockr.rs:361)
🏕 camp site
- Verbose debug logging:
log::debug!fires for every attribute on every page (lockr.rs:322)
📝 note
- Redundant guard is consistent: defensive
rewrite_sdkcheck matches other integrations (lockr.rs:316)
📌 out of scope
- API endpoint domain mismatch (pre-existing): code default
identity.lockr.krvs TOMLidentity.loc.kr— worth a follow-up
🌱 seedling
- SDK mode header: consider
X-Lockr-SDK-Modefor migration observability (lockr.rs:153)
👍 praise
- Test helpers and assertions:
test_config(),test_context(), descriptive messages (lockr.rs:365)
CI Status
- fmt: PASS
- clippy: PASS
- rust tests: PASS
- js tests: PASS
|
Addressed all review feedback in 6f4077a: Blocking — fixed:
Non-blocking — addressed:
|
aram356
left a comment
There was a problem hiding this comment.
Summary
Clean simplification — switches from fragile runtime regex rewriting of obfuscated SDK JavaScript to a dedicated trust-server SDK that handles host routing natively. Net -165 lines. Backwards-compatible deprecation of rewrite_sdk_host is well handled.
Findings
🤔 Medium (P2)
- Operator precedence was a real bug: commit message says "no behavioral change" but the old code matched any
aim.loc.krURL regardless of path (lockr.rs:101) — see inline - Dead code guard in
rewrite():handles_attribute()already gates onrewrite_sdk, so the guard and debug log insiderewrite()never fire (lockr.rs:328) — see inline - Inconsistent log prefix:
[lockr]prefix on debug lines doesn't match rest of file (lockr.rs:337) — see inline - Missing negative test: no Rust test for non-SDK
.jsonidentity.loc.kr(lockr.rs:398) — see inline
⛏ Low (P3)
- Hardcoded domain in doc comment: should say "configured Lockr API endpoint" (lockr.rs:161) — see inline
- Silent default change:
default_api_endpointchanged fromidentity.lockr.krtoidentity.loc.kr(lockr.rs:350) — see inline
👍 Highlights
- Backwards-compatible deprecation of
rewrite_sdk_hostviaOption<bool>+ runtime warning - Operator precedence bug fix in
is_lockr_sdk_url - Test helpers
test_config()/test_context()reduce boilerplate cleanly
CI Status
- fmt: PASS
- clippy: PASS (via Analyze)
- rust tests: PASS
- js tests: PASS
- docs format: PASS
- CodeQL: PASS
- Remove unreachable debug log from rewrite() guard (handles_attribute gates first) - Drop inconsistent [lockr] prefix from debug log to match file conventions - Fix hardcoded domain in doc comment to reference configured endpoint - Add negative test for non-SDK .js files on identity.loc.kr
Addressed remaining review feedback (df8ca54)Thanks for the thorough second pass Aram — all four items are addressed:
Re: the |
Summary
identity-lockr-trust-server.js) which is pre-configured to route API calls through the first-party proxy, eliminating our fragile runtime regex host rewritingrewrite_sdk_hostconfig and regex logic — the new SDK handles this natively, sohandle_sdk_serving()now serves the SDK as-is with no conditional rewriting[lockr]prefix for diagnosing production attribute rewriting issuesChanges
crates/common/src/integrations/lockr.rssdk_urltoidentity-lockr-trust-server.js; removedrewrite_sdk_hostconfig field,rewrite_sdk_host()regex method,default_rewrite_sdk_host()fn, andregeximport; simplifiedhandle_sdk_serving()to serve SDK without host rewriting orX-Lockr-Host-Rewrittenheader; fixed operator precedence inis_lockr_sdk_url()with explicit parentheses; addedtracing::debug!logging toregister()andrewrite(); removed 4 host-rewriting tests, extractedtest_config()/test_context()helpers, addedtest_attribute_rewriter_keeps_non_lockr_urlsand trust-server SDK URL detection testtrusted-server.tomlsdk_urlto match new default (identity-lockr-trust-server.js)Closes
Closes #394
Closes #150
Test plan
cargo test --workspacecargo clippy --all-targets --all-features -- -D warningscargo fmt --all -- --checkcd crates/js/lib && npx vitest runcd crates/js/lib && npm run formatcd docs && npm run formatcargo build --bin trusted-server-fastly --release --target wasm32-wasip1fastly compute serveChecklist
unwrap()in production code — useexpect("should ...")tracingmacros (notprintln!)Backward compatibility
LockrConfigdoes not have#[serde(deny_unknown_fields)], so existing configs withrewrite_sdk_hostwill be silently ignored during deserialization. No breakage.