Skip to content

Update Lockr integration to use dedicated trust-server SDK#395

Open
ChristianPavilonis wants to merge 3 commits intomainfrom
feature/lockr-sdk-update
Open

Update Lockr integration to use dedicated trust-server SDK#395
ChristianPavilonis wants to merge 3 commits intomainfrom
feature/lockr-sdk-update

Conversation

@ChristianPavilonis
Copy link
Collaborator

@ChristianPavilonis ChristianPavilonis commented Mar 3, 2026

Summary

  • Switch to Lockr's dedicated trust-server SDK (identity-lockr-trust-server.js) which is pre-configured to route API calls through the first-party proxy, eliminating our fragile runtime regex host rewriting
  • Remove rewrite_sdk_host config and regex logic — the new SDK handles this natively, so handle_sdk_serving() now serves the SDK as-is with no conditional rewriting
  • Add debug logging with [lockr] prefix for diagnosing production attribute rewriting issues

Changes

File Change
crates/common/src/integrations/lockr.rs Updated default sdk_url to identity-lockr-trust-server.js; removed rewrite_sdk_host config field, rewrite_sdk_host() regex method, default_rewrite_sdk_host() fn, and regex import; simplified handle_sdk_serving() to serve SDK without host rewriting or X-Lockr-Host-Rewritten header; fixed operator precedence in is_lockr_sdk_url() with explicit parentheses; added tracing::debug! logging to register() and rewrite(); removed 4 host-rewriting tests, extracted test_config()/test_context() helpers, added test_attribute_rewriter_keeps_non_lockr_urls and trust-server SDK URL detection test
trusted-server.toml Updated example sdk_url to match new default (identity-lockr-trust-server.js)

Closes

Closes #394
Closes #150

Test plan

  • cargo test --workspace
  • cargo clippy --all-targets --all-features -- -D warnings
  • cargo fmt --all -- --check
  • JS tests: cd crates/js/lib && npx vitest run
  • JS format: cd crates/js/lib && npm run format
  • Docs format: cd docs && npm run format
  • WASM build: cargo build --bin trusted-server-fastly --release --target wasm32-wasip1
  • Manual testing via fastly compute serve
  • Other: N/A

Checklist

  • Changes follow CLAUDE.md conventions
  • No unwrap() in production code — use expect("should ...")
  • Uses tracing macros (not println!)
  • New code has tests
  • No secrets or credentials committed

Backward compatibility

LockrConfig does not have #[serde(deny_unknown_fields)], so existing configs with rewrite_sdk_host will be silently ignored during deserialization. No breakage.

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
@ChristianPavilonis ChristianPavilonis force-pushed the feature/lockr-sdk-update branch from 92cc71c to c2f1452 Compare March 3, 2026 15:34
Copy link
Collaborator

@prk-Jr prk-Jr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me.

@aram356 aram356 linked an issue Mar 5, 2026 that may be closed by this pull request
1 task
Copy link
Collaborator

@aram356 aram356 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.kr URL 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_sdk check matches other integrations (lockr.rs:316)

📌 out of scope

  • API endpoint domain mismatch (pre-existing): code default identity.lockr.kr vs TOML identity.loc.kr — worth a follow-up

🌱 seedling

  • SDK mode header: consider X-Lockr-SDK-Mode for 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

@ChristianPavilonis
Copy link
Collaborator Author

Addressed all review feedback in 6f4077a:

Blocking — fixed:

  • Overly broad aim.loc.kr URL matchingis_lockr_sdk_url() now requires both domains to match identity-lockr and end with .js. Added negative test cases for pixel.gif and styles.css.
  • Silent config drop for rewrite_sdk_host — Re-added as Option<bool> with #[serde(default)] so existing configs deserialize cleanly. register() emits log::warn! when the field is present.

Non-blocking — addressed:

  • Contract coverage — Added test_default_sdk_url_uses_trust_server asserting the default SDK URL contains trust-server.
  • Verbose debug logging — Removed the per-attribute log::debug! that fired on every src/href; now only logs when actually rewriting a Lockr URL.
  • API endpoint domain mismatch — Fixed default_api_endpoint() from identity.lockr.kr to identity.loc.kr to match trusted-server.toml.
  • SDK mode header — Added X-Lockr-SDK-Mode: trust-server to SDK responses for migration observability.

Copy link
Collaborator

@aram356 aram356 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in place one below

Copy link
Collaborator

@aram356 aram356 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.kr URL regardless of path (lockr.rs:101) — see inline
  • Dead code guard in rewrite(): handles_attribute() already gates on rewrite_sdk, so the guard and debug log inside rewrite() 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 .js on identity.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_endpoint changed from identity.lockr.kr to identity.loc.kr (lockr.rs:350) — see inline

👍 Highlights

  • Backwards-compatible deprecation of rewrite_sdk_host via Option<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
@ChristianPavilonis
Copy link
Collaborator Author

Addressed remaining review feedback (df8ca54)

Thanks for the thorough second pass Aram — all four items are addressed:

Finding Resolution
Dead code guard in rewrite() (P2) Removed the unreachable log::debug! line. Kept the guard itself for defensive consistency with other integrations (permutive, gpt, testlight).
Inconsistent [lockr] log prefix (P2) Dropped the [lockr] prefix — now reads "Rewriting Lockr SDK URL to {}" to match the rest of the file.
Missing negative test for non-SDK .js (P2) Added !is_lockr_sdk_url("https://identity.loc.kr/some-other-script.js") assertion.
Hardcoded domain in doc comment (P3) Changed to "forward requests to the configured Lockr API endpoint."

Re: the default_api_endpoint change from identity.lockr.kr — as noted, it was never lockr.kr; the old default was an AI hallucination that never matched the TOML config. The correct domain has always been identity.loc.kr.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants