Update Lockr integration to use dedicated trust-server SDK#395
Update Lockr integration to use dedicated trust-server SDK#395ChristianPavilonis wants to merge 1 commit 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
ChristianPavilonis
force-pushed
the
feature/lockr-sdk-update
branch
from
92cc71c to
c2f1452
Compare
aram356
left a comment
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
| @@ -103,48 +92,22 @@ impl LockrIntegration { | |||
| fn is_lockr_sdk_url(&self, url: &str) -> bool { | |||
| let lower = url.to_ascii_lowercase(); | |||
| lower.contains("aim.loc.kr") | |||
There was a problem hiding this comment.
🔧 wrench — aim.loc.kr branch matches any URL on that domain (images, CSS, etc.) without requiring identity-lockr or .js. The identity.loc.kr branch correctly applies both guards, but aim.loc.kr does not.
For example, <img src="https://aim.loc.kr/pixel.gif"> would be rewritten to the SDK proxy endpoint.
Fix: Apply the same specificity to both branches:
fn is_lockr_sdk_url(&self, url: &str) -> bool {
let lower = url.to_ascii_lowercase();
(lower.contains("aim.loc.kr") || lower.contains("identity.loc.kr"))
&& lower.contains("identity-lockr")
&& lower.ends_with(".js")
}Also add negative test cases:
assert!(!integration.is_lockr_sdk_url("https://aim.loc.kr/pixel.gif"));
assert!(!integration.is_lockr_sdk_url("https://aim.loc.kr/styles.css"));| @@ -64,10 +57,6 @@ pub struct LockrConfig { | |||
| #[serde(default = "default_rewrite_sdk")] | |||
| pub rewrite_sdk: bool, | |||
There was a problem hiding this comment.
🔧 wrench — The removed rewrite_sdk_host field (previously just below this line) will be silently ignored by serde in existing configs. Deployments that still use identity-lockr-v1.0.js with rewrite_sdk_host = true will silently lose host rewriting — API calls from the SDK will bypass the first-party /integrations/lockr/api proxy and go directly to Lockr's servers.
Fix (either):
- (a) Add a deprecated field that warns at startup:
/// Deprecated — the trust-server SDK handles host routing natively.
#[serde(default)]
rewrite_sdk_host: Option<bool>,Then in register():
if integration.config.rewrite_sdk_host.is_some() {
log::warn!("lockr: `rewrite_sdk_host` is deprecated and ignored; \
the trust-server SDK handles host routing natively");
}- (b) At minimum, warn when
sdk_urldoes not containtrust-server:
if !integration.config.sdk_url.contains("trust-server") {
log::warn!("lockr: sdk_url does not appear to be a trust-server SDK; \
API calls may bypass first-party routing");
}| true | ||
| } | ||
|
|
||
| #[cfg(test)] |
There was a problem hiding this comment.
🤔 thinking — The old host-rewriting tests were correctly removed, but no replacement test asserts the new contract: that the SDK is served unmodified and default_sdk_url() contains trust-server. A simple assertion would document this assumption and prevent a future default URL change from silently breaking first-party routing.
| // Rewrite to first-party SDK endpoint | ||
| AttributeRewriteAction::Replace(format!( | ||
| let is_lockr = self.is_lockr_sdk_url(attr_value); | ||
| log::debug!( |
There was a problem hiding this comment.
🏕 camp site — This log::debug! runs for every src/href attribute on every HTML page, even for non-Lockr URLs. Consider log::trace! or only logging when is_lockr is true. Other integrations (datadome, gpt, permutive) don't log at this granularity in rewrite().
| @@ -364,22 +314,30 @@ impl IntegrationAttributeRewriter for LockrIntegration { | |||
| ctx: &IntegrationAttributeContext<'_>, | |||
| ) -> AttributeRewriteAction { | |||
| if !self.config.rewrite_sdk { | |||
There was a problem hiding this comment.
📝 note — This rewrite_sdk guard is redundant since handles_attribute() already checks it, but this defensive pattern is used consistently across other integrations (permutive, gpt, testlight). No action needed — just noting for awareness.
| "X-Lockr-Host-Rewritten", | ||
| self.config.rewrite_sdk_host.to_string(), | ||
| ) | ||
| .with_body(sdk_body)) |
There was a problem hiding this comment.
🌱 seedling — The X-Lockr-Host-Rewritten header removal is correct since the feature is gone. For migration observability, consider adding X-Lockr-SDK-Mode: trust-server so operators can confirm which SDK variant is being served.
| #[test] | ||
| fn test_lockr_sdk_url_detection() { | ||
| let config = LockrConfig { | ||
| fn test_config() -> LockrConfig { |
There was a problem hiding this comment.
👍 praise — Nice extraction of test_config() and test_context() helpers, descriptive "should ..." assertion messages, and the new test_attribute_rewriter_keeps_non_lockr_urls test. Follows project testing conventions well.
3 participants
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.