Repository navigation
feat(push): add the push relay that sends the phone app its boxes' notifications - #208
Merged
Merged
Conversation
…tifications Apple delivers a notification only from a server holding the app's push key, which a box can't have. The relay, for push.berthd.app, seals a phone's device token into a key only it can open, takes a box's notice with that key, and makes the notification itself from a few checked fields. It keeps nothing, and logs no token, key or text. Built with esbuild into Vercel's Build Output API, checked in CI. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
12 issues found across 17 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="push/src/schema.ts">
<violation number="1" location="push/src/schema.ts:55">
P2: `checkNotice` accepts a `kind: "session"` notice without `session`, but the APNs payload then has no session target to open. Require `session` whenever `kind` is `session`.</violation>
</file>
<file name="push/README.md">
<violation number="1" location="push/README.md:7">
P3: This source reference is broken: `internal/box/phonev2.go` does not exist, so readers cannot reach the box-side implementation. Point it to `internal/box/phone.go`.</violation>
</file>
<file name="push/package.json">
<violation number="1" location="push/package.json:7">
P2: The declared Node range is broader than the test script supports. Raise the minimum to Node 22.6.0 or use a test runner that supports the full `>=22` range.</violation>
</file>
<file name="push/src/ratelimit.ts">
<violation number="1" location="push/src/ratelimit.ts:31">
P1: This cleanup does not bound memory or work: a stream of distinct IDs that remain within the window makes `hits` grow without limit and makes every subsequent request scan the whole map. Because callers can supply many distinct addresses or keys, an attacker can exhaust a warm function’s memory or CPU; enforce a hard bounded cache with an explicit eviction policy and bounded cleanup work.</violation>
</file>
<file name="push/test/relay.test.ts">
<violation number="1" location="push/test/relay.test.ts:136">
P2: This assertion does not verify that `BadDeviceToken` marks the device as gone. Assert the full outcome so the test catches a regression from the documented 410 path to a generic 502.</violation>
<violation number="2" location="push/test/relay.test.ts:142">
P2: The retry assertion does not verify that the second request uses a fresh provider token. Compare the two authorization headers so a retry with the expired bearer token cannot pass this test.</violation>
</file>
<file name="push/src/relay.ts">
<violation number="1" location="push/src/relay.ts:35">
P2: This check accepts inherited property names such as `constructor` or `toString` as configured sealing keys. Reject missing keys using an own-property check so a bad `SEAL_KID` produces the intended configuration error rather than making every registration fail.</violation>
</file>
<file name="push/src/node.ts">
<violation number="1" location="push/src/node.ts:23">
P2: The cap-crossing chunk is discarded, so a chunked request can append arbitrary data while the valid prefix is still accepted. Preserve enough of the crossing chunk to make the forwarded body exceed `MAX_BODY`, allowing `relay` to return 413.</violation>
<violation number="2" location="push/src/node.ts:29">
P2: `TRACE` requests reach this handler, but Node's `Request` constructor rejects that method before `relay` can return 405. Guard unsupported methods before constructing the web request.</violation>
</file>
<file name="push/src/apns.ts">
<violation number="1" location="push/src/apns.ts:81">
P1: Concurrent expired-token responses can mint one fresh JWT per notice here. That violates APNs’ 20-minute token-renewal limit and can make all but the first retry fail; coalesce forced refreshes and reuse one replacement token.</violation>
<violation number="2" location="push/src/apns.ts:114">
P2: An old session can remove a newer session from `sessions` during reconnect, causing repeated connection creation and violating the one-session-per-host behavior. Delete the map entry only when it still points to the session emitting the event.</violation>
</file>
<file name=".github/workflows/ci.yml">
<violation number="1" location=".github/workflows/ci.yml:318">
P2: This checkout persists `GITHUB_TOKEN` while the job executes pull-request-controlled scripts. Set `persist-credentials: false` so a fork cannot read and exfiltrate the token from the checkout.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| if (ok) kept.push(now); | ||
| this.hits.set(id, kept); | ||
| // Forget ids that went quiet, so the map stays small. | ||
| if (this.hits.size > 10_000) for (const [k, v] of this.hits) if (!v.length || now - v[v.length - 1] >= longest) this.hits.delete(k); |
There was a problem hiding this comment.
P1: This cleanup does not bound memory or work: a stream of distinct IDs that remain within the window makes hits grow without limit and makes every subsequent request scan the whole map. Because callers can supply many distinct addresses or keys, an attacker can exhaust a warm function’s memory or CPU; enforce a hard bounded cache with an explicit eviction policy and bounded cleanup work.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At push/src/ratelimit.ts, line 31:
<comment>This cleanup does not bound memory or work: a stream of distinct IDs that remain within the window makes `hits` grow without limit and makes every subsequent request scan the whole map. Because callers can supply many distinct addresses or keys, an attacker can exhaust a warm function’s memory or CPU; enforce a hard bounded cache with an explicit eviction policy and bounded cleanup work.</comment>
<file context>
@@ -0,0 +1,34 @@
+ if (ok) kept.push(now);
+ this.hits.set(id, kept);
+ // Forget ids that went quiet, so the map stays small.
+ if (this.hits.size > 10_000) for (const [k, v] of this.hits) if (!v.length || now - v[v.length - 1] >= longest) this.hits.delete(k);
+ return ok;
+ }
</file context>
| body: JSON.stringify(payload(n)), | ||
| }); | ||
| let res = await this.transport(req(false)); | ||
| if (res.status === 403 && reasonOf(res.body) === "ExpiredProviderToken") res = await this.transport(req(true)); |
There was a problem hiding this comment.
P1: Concurrent expired-token responses can mint one fresh JWT per notice here. That violates APNs’ 20-minute token-renewal limit and can make all but the first retry fail; coalesce forced refreshes and reuse one replacement token.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At push/src/apns.ts, line 81:
<comment>Concurrent expired-token responses can mint one fresh JWT per notice here. That violates APNs’ 20-minute token-renewal limit and can make all but the first retry fail; coalesce forced refreshes and reuse one replacement token.</comment>
<file context>
@@ -0,0 +1,134 @@
+ body: JSON.stringify(payload(n)),
+ });
+ let res = await this.transport(req(false));
+ if (res.status === 403 && reasonOf(res.body) === "ExpiredProviderToken") res = await this.transport(req(true));
+ if (res.status === 200) return { ok: true };
+ const reason = reasonOf(res.body);
</file context>
| const session = v.session === undefined ? undefined : text(v.session, 120, NAME); | ||
| const run = v.run === undefined ? undefined : text(v.run, 120, NAME); | ||
| if ((v.session !== undefined && !session) || (v.run !== undefined && !run)) return { ok: false, error: "session and run are names" }; | ||
| if (v.kind === "run" && !run) return { ok: false, error: "a run's notice names the run" }; |
There was a problem hiding this comment.
P2: checkNotice accepts a kind: "session" notice without session, but the APNs payload then has no session target to open. Require session whenever kind is session.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At push/src/schema.ts, line 55:
<comment>`checkNotice` accepts a `kind: "session"` notice without `session`, but the APNs payload then has no session target to open. Require `session` whenever `kind` is `session`.</comment>
<file context>
@@ -0,0 +1,69 @@
+ const session = v.session === undefined ? undefined : text(v.session, 120, NAME);
+ const run = v.run === undefined ? undefined : text(v.run, 120, NAME);
+ if ((v.session !== undefined && !session) || (v.run !== undefined && !run)) return { ok: false, error: "session and run are names" };
+ if (v.kind === "run" && !run) return { ok: false, error: "a run's notice names the run" };
+ return { ok: true, value: { v: 1, kind: v.kind, state: v.state, title, body, box, session, run, collapse } };
+}
</file context>
Suggested change
| if (v.kind === "run" && !run) return { ok: false, error: "a run's notice names the run" }; | |
| if ((v.kind === "session" && !session) || (v.kind === "run" && !run)) return { ok: false, error: "a notice names its session or run" }; |
| "description": "Shipyard's push relay: sends a phone app the notifications its boxes ask for. Deployed to push.berthd.app.", | ||
| "type": "module", | ||
| "engines": { | ||
| "node": ">=22" |
There was a problem hiding this comment.
P2: The declared Node range is broader than the test script supports. Raise the minimum to Node 22.6.0 or use a test runner that supports the full >=22 range.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At push/package.json, line 7:
<comment>The declared Node range is broader than the test script supports. Raise the minimum to Node 22.6.0 or use a test runner that supports the full `>=22` range.</comment>
<file context>
@@ -0,0 +1,19 @@
+ "description": "Shipyard's push relay: sends a phone app the notifications its boxes ask for. Deployed to push.berthd.app.",
+ "type": "module",
+ "engines": {
+ "node": ">=22"
+ },
+ "scripts": {
</file context>
Suggested change
| "node": ">=22" | |
| "node": ">=22.6.0" |
| let a = answers([{ status: 410, body: '{"reason":"Unregistered"}' }]); | ||
| assert.deepEqual(await a.apns.send({ token: TOKEN, env: "production" }, notice), { ok: false, gone: true, reason: "Unregistered" }); | ||
| a = answers([{ status: 400, body: '{"reason":"BadDeviceToken"}' }]); | ||
| assert.equal((await a.apns.send({ token: TOKEN, env: "production" }, notice)).ok, false); |
There was a problem hiding this comment.
P2: This assertion does not verify that BadDeviceToken marks the device as gone. Assert the full outcome so the test catches a regression from the documented 410 path to a generic 502.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At push/test/relay.test.ts, line 136:
<comment>This assertion does not verify that `BadDeviceToken` marks the device as gone. Assert the full outcome so the test catches a regression from the documented 410 path to a generic 502.</comment>
<file context>
@@ -0,0 +1,244 @@
+ let a = answers([{ status: 410, body: '{"reason":"Unregistered"}' }]);
+ assert.deepEqual(await a.apns.send({ token: TOKEN, env: "production" }, notice), { ok: false, gone: true, reason: "Unregistered" });
+ a = answers([{ status: 400, body: '{"reason":"BadDeviceToken"}' }]);
+ assert.equal((await a.apns.send({ token: TOKEN, env: "production" }, notice)).ok, false);
+ a = answers([
+ { status: 403, body: '{"reason":"ExpiredProviderToken"}' },
</file context>
Suggested change
| assert.equal((await a.apns.send({ token: TOKEN, env: "production" }, notice)).ok, false); | |
| assert.deepEqual(await a.apns.send({ token: TOKEN, env: "production" }, notice), { ok: false, gone: true, reason: "BadDeviceToken" }); |
| const headers = new Headers(); | ||
| for (const [k, v] of Object.entries(req.headers)) if (typeof v === "string") headers.set(k, v); | ||
| const method = req.method ?? "GET"; | ||
| const request = new Request(`https://${req.headers.host ?? "push.berthd.app"}${req.url ?? "/"}`, { |
There was a problem hiding this comment.
P2: TRACE requests reach this handler, but Node's Request constructor rejects that method before relay can return 405. Guard unsupported methods before constructing the web request.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At push/src/node.ts, line 29:
<comment>`TRACE` requests reach this handler, but Node's `Request` constructor rejects that method before `relay` can return 405. Guard unsupported methods before constructing the web request.</comment>
<file context>
@@ -0,0 +1,39 @@
+ const headers = new Headers();
+ for (const [k, v] of Object.entries(req.headers)) if (typeof v === "string") headers.set(k, v);
+ const method = req.method ?? "GET";
+ const request = new Request(`https://${req.headers.host ?? "push.berthd.app"}${req.url ?? "/"}`, {
+ method,
+ headers,
</file context>
| let size = 0; | ||
| for await (const c of req) { | ||
| size += (c as Buffer).length; | ||
| if (size > MAX_BODY + 1) break; |
There was a problem hiding this comment.
P2: The cap-crossing chunk is discarded, so a chunked request can append arbitrary data while the valid prefix is still accepted. Preserve enough of the crossing chunk to make the forwarded body exceed MAX_BODY, allowing relay to return 413.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At push/src/node.ts, line 23:
<comment>The cap-crossing chunk is discarded, so a chunked request can append arbitrary data while the valid prefix is still accepted. Preserve enough of the crossing chunk to make the forwarded body exceed `MAX_BODY`, allowing `relay` to return 413.</comment>
<file context>
@@ -0,0 +1,39 @@
+ let size = 0;
+ for await (const c of req) {
+ size += (c as Buffer).length;
+ if (size > MAX_BODY + 1) break;
+ chunks.push(c as Buffer);
+ }
</file context>
Comment on lines
+114
to
+115
| fresh.on("error", () => sessions.delete(host)); | ||
| fresh.on("close", () => sessions.delete(host)); |
There was a problem hiding this comment.
P2: An old session can remove a newer session from sessions during reconnect, causing repeated connection creation and violating the one-session-per-host behavior. Delete the map entry only when it still points to the session emitting the event.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At push/src/apns.ts, line 114:
<comment>An old session can remove a newer session from `sessions` during reconnect, causing repeated connection creation and violating the one-session-per-host behavior. Delete the map entry only when it still points to the session emitting the event.</comment>
<file context>
@@ -0,0 +1,134 @@
+ const s = sessions.get(host);
+ if (s && !s.closed && !s.destroyed) return s;
+ const fresh = connect(host);
+ fresh.on("error", () => sessions.delete(host));
+ fresh.on("close", () => sessions.delete(host));
+ // Not kept alive for the connection alone: a function may freeze.
</file context>
Suggested change
| fresh.on("error", () => sessions.delete(host)); | |
| fresh.on("close", () => sessions.delete(host)); | |
| const drop = () => { | |
| if (sessions.get(host) === fresh) sessions.delete(host); | |
| }; | |
| fresh.on("error", drop); | |
| fresh.on("close", drop); |
| echo "::error::$EVENT by $ACTOR may not run on a self-hosted runner" | ||
| exit 1 | ||
| fi | ||
| - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 |
There was a problem hiding this comment.
P2: This checkout persists GITHUB_TOKEN while the job executes pull-request-controlled scripts. Set persist-credentials: false so a fork cannot read and exfiltrate the token from the checkout.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At .github/workflows/ci.yml, line 318:
<comment>This checkout persists `GITHUB_TOKEN` while the job executes pull-request-controlled scripts. Set `persist-credentials: false` so a fork cannot read and exfiltrate the token from the checkout.</comment>
<file context>
@@ -291,3 +291,43 @@ jobs:
+ echo "::error::$EVENT by $ACTOR may not run on a self-hosted runner"
+ exit 1
+ fi
+ - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
+ - uses: pnpm/action-setup@b906affcce14559ad1aafd4ab0e942779e9f58b1 # v4.3.0
+ with:
</file context>
Suggested change
| - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 | |
| - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 | |
| with: | |
| persist-credentials: false |
| `push.berthd.app`. Apple delivers a notification only from a server holding | ||
| the app's push key, which a box can't have, so boxes ask this relay. How it | ||
| fits together is in [the phone guide](../docs/guides/phone.mdx); the box's side is | ||
| `internal/box/phonev2.go`. |
There was a problem hiding this comment.
P3: This source reference is broken: internal/box/phonev2.go does not exist, so readers cannot reach the box-side implementation. Point it to internal/box/phone.go.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At push/README.md, line 7:
<comment>This source reference is broken: `internal/box/phonev2.go` does not exist, so readers cannot reach the box-side implementation. Point it to `internal/box/phone.go`.</comment>
<file context>
@@ -0,0 +1,58 @@
+`push.berthd.app`. Apple delivers a notification only from a server holding
+the app's push key, which a box can't have, so boxes ask this relay. How it
+fits together is in [the phone guide](../docs/guides/phone.mdx); the box's side is
+`internal/box/phonev2.go`.
+
+It keeps nothing. A phone app registers its device token and gets back a
</file context>
Suggested change
| `internal/box/phonev2.go`. | |
| `internal/box/phone.go`. |
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Third of the phone app's PRs (independent of #206 and the box PR). Apple delivers a notification only from a server holding the app's APNs key, which a box can't have, so
push/is a small relay forpush.berthd.app.How it works (
push/README.md)POST /v1/register {platform, token, env, bundle}seals the device token (AES-256-GCM, by key id so keys can rotate) into a key only the relay opens. The app gives the key to its boxes; boxes never see the token.POST /v1/sendwithBearer <key>and a notice: a strict schema (fixed kind and state, short single-line text, no extra fields), and the relay builds the APNs payload itself. Token-based APNs auth (ES256 JWT, renewed every 50 minutes, retried once onExpiredProviderToken) over HTTP/2, one connection per host.Build: no runtime dependencies. esbuild bundles
src/node.tsinto Vercel's Build Output API layout (.vercel/output, one Node 22 function), so the deploy is exactly what CI built, without relying on how Vercel compiles.tsimports.To deploy: a Vercel project with root
push/, the domain, andAPNS_KEY_P8,APNS_KEY_ID,APNS_TEAM_ID,APNS_TOPIC=dev.berth.phone,SEAL_KEYS,SEAL_KID(see the README). Not deployed by this PR.Checks:
pnpm typecheck && pnpm test && pnpm build(newpushCI job). 12 tests cover sealing, the schema, limits, the JWT and payload, APNs answers, the HTTP/2 transport against a local server, and the routes end to end. The bundled function was also smoke-tested locally (health, register, 401, 413).🤖 Generated with Claude Code