Repository navigation
fix(api): default box visibility to private - #1370
Merged
DefeatMan merged 3 commits intoAug 28, 2026
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughBox creation defaults fresh and warm-pool boxes to private visibility when unspecified. The create-box controller no longer sets anonymous-public warning headers. Tunnel requests for private boxes now return a conflict before URL resolution. ChangesPrivate visibility defaults
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: 🟡 Moderate · up to The PR changes omitted visibility to private and adds checks before issuing tunnel URLs, but a recently public box can remain anonymously reachable for up to three seconds after being changed to private because downstream public-status caching is not invalidated. This bounded security window should be fixed or explicitly accepted before merge. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description provides the issue reference, before-and-after call paths, change rationale, test coverage, verification steps, and breaking-change risk. It omits some template headings, but the required information is mostly present.
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
G4614
force-pushed
the
pol-205-anonymous-public-warning
branch
from
August 28, 2026 07:39
270a956 to
3a66143
Compare
G4614
marked this pull request as ready for review
August 28, 2026 08:05
📦 BoxLite review — couldn't completepowered by BoxLite |
G4614
changed the title
Phase 1 of POL-205. A caller who never mentions network.inbound at create time gets an anonymously-public box via the `?? true` fallback in box.service.ts, with no signal that this happened — the API just returns 201. Add isAnonymouslyPublicByDefault() (box-to-box.mapper.ts) to detect exactly that case — the request had no explicit inbound.mode, not an opt-in — and set a Warning response header on box creation so the gap is visible before the default itself changes. Does not touch the default value or the three-state inbound.mode design (token/public/disabled) from POL-205's Phase 2 — this is only the visibility half, per the issue's own migration plan.
Fixes POL-205. Boxes were reachable from the public internet by default: an unspecified network.inbound fell through to `createBoxDto.public ?? true` in both the fresh-box and warm-pool-assignment paths, with no way to discover this short of probing the box's own tunnel URL. Flips both defaults to false. Supersedes the Warning-header mitigation from the prior commit on this branch — once the default is private, a caller can no longer end up anonymously public without asking, so the header served no further purpose.
G4614
force-pushed
the
pol-205-anonymous-public-warning
branch
from
August 28, 2026 12:22
95148a5 to
64d3878
Compare
Private boxes (the new default) reject tunnel URL requests with 409. Callers must explicitly set public: true before a tunnel URL is issued. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
DefeatMan
added a commit
to DefeatMan/boxlite
that referenced
this pull request
Sep 22, 2026
Running this suite against api.dev.boxlite.ai found three defects in what this branch had just added, and three areas the cloud path never covered. The sweep selected by idle time alone: a report against dev listed five of a colleague's POL-599 boxes as candidates. Boxes the cases create are now named `e2e-<random>` and only that prefix is swept; `--any-name` is the opt-out and an empty prefix is refused. The sweep also waits for a box to stop being served, and only a 404 says it is gone. Completing one lifecycle window beside a caller's other one could build a pair the SDK rejects, so the SDK door applies both or neither. A hand-built REST body meets no such rule and is completed and named. The workflow now refuses a stage outside dev/prod and a non-https API_URL. The Node tunnel driver only stopped its four boxes; it removes them now. Ported from the local suite, none of it reachable there before: the box's main command, secrets, and a read-only mount refusal. Tunnel and preview become xfail(strict) — since boxlite-ai#1370 no SDK caller can create a public box, and raw REST with the nested shape shows that is the client's fault, not the server's. test_harness_guards.py covers the suite's own rules without a stage, each one run first against the defect it reproduces. What is not verified is recorded in apps/e2e/README.md: the volume cases skip on dev, whose key holds no volume permission, and short-exec stdout is dropped there until boxlite-ai#1569 lands.
DefeatMan
added a commit
to DefeatMan/boxlite
that referenced
this pull request
Sep 23, 2026
Every CLI case in the suite detaches, so `boxlite run <image> <cmd>`
without `-d` was never exercised — and on a cloud stage it fails every
time. That form is a different path, not a nicer spelling of the same
one: create → WS `/boxes/{id}/attach` → `POST /start`, and that
upgrade comes back a bare 503 whose body (`upstream connect error or
disconnect/reset before headers`) is an envelope the API never writes.
So the refusal comes from the load balancing in front of it; which hop
drops the upgrade is not established. A green suite said nothing about
any of it.
The case is `xfail(strict=True)`, matching how the tunnel cases carry
boxlite-ai#1370, but conditional on a remote stage host where those are
unconditional. The cause is stage topology, not product code: a local
stack reaches boxlite-api on :3000 with no load balancer in front of
it, so an unconditional mark would turn a local pass into an XPASS
failure and take `make test:e2e` red. What a local stack does with the
attached form is untested, and the mark claims nothing about it.
`--runxfail` confirms the failure signal against dev is the 503 itself,
not a parse or setup error.
The create lands before the 503, so a failed run still costs a box and
never prints its id. The case therefore names its box up front and
removes it by name in `finally`; three runs against dev left nothing
behind. `--rm` cannot do that job — it conflicts with the lifecycle
deadlines, which are what reclaims the box if the removal itself
fails. That opt-in is also the one exception to conftest's note that
CLI boxes are beyond sweep.py's reach, so the note now says so.
DefeatMan
added a commit
to DefeatMan/boxlite
that referenced
this pull request
Sep 24, 2026
Running this suite against api.dev.boxlite.ai found three defects in what this branch had just added, and three areas the cloud path never covered. The sweep selected by idle time alone: a report against dev listed five of a colleague's POL-599 boxes as candidates. Boxes the cases create are now named `e2e-<random>` and only that prefix is swept; `--any-name` is the opt-out and an empty prefix is refused. The sweep also waits for a box to stop being served, and only a 404 says it is gone. Completing one lifecycle window beside a caller's other one could build a pair the SDK rejects, so the SDK door applies both or neither. A hand-built REST body meets no such rule and is completed and named. The workflow now refuses a stage outside dev/prod and a non-https API_URL. The Node tunnel driver only stopped its four boxes; it removes them now. Ported from the local suite, none of it reachable there before: the box's main command, secrets, and a read-only mount refusal. Tunnel and preview become xfail(strict) — since boxlite-ai#1370 no SDK caller can create a public box, and raw REST with the nested shape shows that is the client's fault, not the server's. test_harness_guards.py covers the suite's own rules without a stage, each one run first against the defect it reproduces. What is not verified is recorded in apps/e2e/README.md: the volume cases skip on dev, whose key holds no volume permission, and short-exec stdout is dropped there until boxlite-ai#1569 lands.
DefeatMan
added a commit
to DefeatMan/boxlite
that referenced
this pull request
Sep 24, 2026
Every CLI case in the suite detaches, so `boxlite run <image> <cmd>`
without `-d` was never exercised — and on a cloud stage it fails every
time. That form is a different path, not a nicer spelling of the same
one: create → WS `/boxes/{id}/attach` → `POST /start`, and that
upgrade comes back a bare 503 whose body (`upstream connect error or
disconnect/reset before headers`) is an envelope the API never writes.
So the refusal comes from the load balancing in front of it; which hop
drops the upgrade is not established. A green suite said nothing about
any of it.
The case is `xfail(strict=True)`, matching how the tunnel cases carry
boxlite-ai#1370, but conditional on a remote stage host where those are
unconditional. The cause is stage topology, not product code: a local
stack reaches boxlite-api on :3000 with no load balancer in front of
it, so an unconditional mark would turn a local pass into an XPASS
failure and take `make test:e2e` red. What a local stack does with the
attached form is untested, and the mark claims nothing about it.
`--runxfail` confirms the failure signal against dev is the 503 itself,
not a parse or setup error.
The create lands before the 503, so a failed run still costs a box and
never prints its id. The case therefore names its box up front and
removes it by name in `finally`; three runs against dev left nothing
behind. `--rm` cannot do that job — it conflicts with the lifecycle
deadlines, which are what reclaims the box if the removal itself
fails. That opt-in is also the one exception to conftest's note that
CLI boxes are beyond sweep.py's reach, so the note now says so.
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
2 participants
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.
Fixes POL-205.
Before
Same bug in
assignWarmPoolBox(box.service.ts:355) for boxes served fromthe warm pool.
After
This supersedes the Warning-header mitigation from the branch's prior commit —
once the default is private, a caller can no longer end up anonymously public
without asking, so the header (and its
isAnonymouslyPublicByDefaulthelper)served no further purpose and were removed.
Note: this is a breaking behavior change for any existing caller relying on
the old anonymous-public default. Flagging for release notes / staged rollout
per the issue's own migration plan.
Test plan
box.service.spec.ts's existingpublic defaultstests pin both callsites; updated their expected values for the new default (undefined -> false,
explicit true -> true) — 682 passed, 72 skipped, 0 failed on the full
apisuite
?? falselines back to?? true,confirmed both pinning tests fail with the expected "expect false, got true"
diff; restored, all green again
tsc --noEmitclean🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes