feat(NODE-1978): Custom Domains bypass logic - #113
Conversation
There was a problem hiding this comment.
Pull request overview
Adds authenticated canister-ID overrides for custom-domain registration and updates while retaining standard DNS validation as fallback.
Changes:
- Adds Bearer-token bypass handling and limited validation.
- Propagates override IDs through API, canister state, and workers.
- Adds unit and end-to-end coverage.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
Cargo.toml |
Enables typed headers. |
Cargo.lock |
Locks new header dependencies. |
ic-bn-lib/src/http/middleware/rate_limiter.rs |
Supports Authorization bypass tokens. |
ic-bn-lib/src/custom_domains/tests/helpers.rs |
Updates task fixtures. |
ic-bn-lib/src/custom_domains/tests/e2e_test.rs |
Tests bypass registration end-to-end. |
ic-bn-lib/src/custom_domains/client.rs |
Maps override IDs into worker tasks. |
ic-bn-lib/src/custom_domains/base/types/worker.rs |
Uses limited validation for overrides. |
ic-bn-lib/src/custom_domains/base/types/validator.rs |
Implements limited validation. |
ic-bn-lib/src/custom_domains/base/types/task.rs |
Adds task canister IDs. |
ic-bn-lib/src/custom_domains/base/traits/validation.rs |
Extends the validation contract. |
ic-bn-lib/src/custom_domains/backend/router.rs |
Configures bypass-aware services and tests. |
ic-bn-lib/src/custom_domains/backend/handlers.rs |
Accepts authenticated query overrides. |
ic-bn-lib/src/custom_domains/backend/backend_service.rs |
Selects full or limited validation. |
custom-domains/canister/src/state.rs |
Persists per-task override IDs. |
custom-domains/canister/canister_backend.did |
Extends the Candid task schema. |
custom-domains/api/src/lib.rs |
Extends Rust API task types. |
Suppressed comments (2)
ic-bn-lib/src/custom_domains/backend/handlers.rs:66
- This optional typed extractor still rejects malformed or non-Bearer
Authorizationheaders before the handler runs, rather than treating them as a missing/incorrect bypass token. It therefore breaks the documented silent fallback (and previously valid requests carrying unrelated authorization). Extract the header as a fallible/raw value and map parse failures to no bypass.
authorization: Option<TypedHeader<Authorization<Bearer>>>,
ic-bn-lib/src/custom_domains/backend/handlers.rs:134
- As in the create handler, a malformed or non-Bearer
Authorizationheader is rejected by this extractor before the update can fall back to standard validation. Treat extraction/parsing failure as an absent bypass token to preserve the documented fallback behavior.
authorization: Option<TypedHeader<Authorization<Bearer>>>,
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
ic-bn-lib/src/http/middleware/rate_limiter.rs:125
- The legacy header suppresses
Authorizationwhenever it is present, even when its value is wrong, so a request with a valid Bearer token plus a stale/incorrect legacy header is still rate-limited. Also,trim_start_matchestreats a non-Bearer authorization value as the token. Check the two headers independently, require a Bearer scheme (case-insensitively), and bypass when either check succeeds.
.get(BYPASS_TOKEN_HEADER)
.map(|x| x.as_bytes())
.or_else(|| {
request
.headers()
.get(AUTHORIZATION)
.and_then(|v| v.to_str().ok())
.map(|s| s.trim_start_matches("Bearer ").as_bytes())
ic-bn-lib/src/custom_domains/backend/handlers.rs:30
- Deserializing the query directly as
Principalhappens before the token check. Consequently, an unauthenticated request such as?canister_id=invalidnow fails with 400, whereas the stated fallback behavior requires the canister parameter to be ignored when the bypass token is missing or incorrect (and this parameter was ignored before this change). Keep the raw value in the query and parse it only after authenticating the bypass token; apply the same deferred parsing to POST and PATCH.
/// Canister ID to associate the domain with.
/// Taken into account only if the bypass token is provided in the request.
pub canister_id: Option<Principal>,
| if let (TaskKind::Issue | TaskKind::Update, Some(canister_id)) = | ||
| (task.kind, task.canister_id) | ||
| { | ||
| entry.bypass_validation = true; | ||
| entry.canister_id = Some(canister_id); |
There was a problem hiding this comment.
This is ok since the limited validation doesn't care about canister id
/.well-known/ic-domainsvalidation)bypass_validationfield to the canister to indicate that the provided canister ID is to be trusted (backwards compatible with stable structure)x-ratelimit-bypass-token(which only bypasses rate-limit) andAuthorization: Bearerheaders (which allows setting canister ID & bypasses rate limiter at the same time). In the future they should use theAuthorizationheader.