Stop a pre-existing approval reading as tampering #355

Merged

branderson merged 1 commits from approval-fix into development 2026-09-15 19:46:39 -07:00

Conversation 1 Commits 1 Files Changed 5

+85 -21

dejima-agent commented 2026-09-14 22:41:58 -07:00

Collaborator

Copy Link

Copy Source

Summary generated by LLM agent

Two holes at either end of the two-label rule this release adds. The first is the most serious defect the review has turned up: it makes every launch fail for an operator whose only action was to upgrade.

A pre-existing approval now reads as tampering

The rule was applied where approvals are read. approved_entries validates every entry on every read, and bin/dejima assigns from it under set -euo pipefail about 1200 lines before the launch check. So a single-label domain already in the store fails there:

store contains "loki"
  before:  exit=0, launch proceeds
  after:   corrupt approval entry in .../allowed-domains: loki
           exit=1, docker never invoked

Three things make this worse than an ordinary refusal.

The global store is read on every launch, so this is not one project failing - it is every project, until the file is edited by hand. dejima-allow has no remove verb, so that is the only recovery.

The message is corrupt approval entry, and approved_entries' own comment says such an entry "means a bug or tampering, not user error". It would be saying that about the operator's own file, on every launch, for a value that was valid when they approved it. The last release accepted single-label domains - a commit in this very release said so explicitly while adding support for them, before that support was reverted - so this is the store an upgrading operator has.

And the launch-time message written for exactly this case is unreachable behind it. The careful error naming the key and explaining the rule is dead code; what an operator actually meets is the tampering message.

Granting is where a tightened rule belongs. Reading keeps the shape check it had, which still refuses genuine corruption - a malformed entry like a..b.com fails the read as before. approve_entry refuses a single-label domain when it is granted, with a message that says why, and dejima-project classifies one in a project's declaration as rejected.

The launch check counted dots, and both slips it was written for have one

Its stated purpose was to catch a host derivation that went wrong, on the grounds that a sed over a URL can leave something nobody typed. It only caught that when the residue happened to be dotless:

https://user@host.example/   ->  user@host.example    was accepted
http://loki./                ->  loki.                was accepted
http://user:pw@llm.local/    ->  user                 was refused

The first two reach the container and are refused there instead, which is the failure this check exists to move. It now asks the same question of the value that the policy will ask - a host name, of at least two labels - rather than a cheaper one that happens to agree in one case out of three.

Its message changed shape with that, so the assertion that named the old wording now names the rule instead.

What was checked and found sound

The same pass drove the check for over-refusal and found none: IPv4 addresses, an IPv4 in internal_domains, punycode, uppercase, empty and unset values, and odd list whitespace all pass, and everything it refuses that another layer accepts was already refused at container start. Placement is right - --help, --print-config and --print-briefing all exit before it, so a diagnostic still works when a launch does not. All five sources are final at the point it runs, and none is modified afterwards.

Testing

Full confined gate, all ten suites, green.

Both fixes were run against their own removal: restoring the narrow validator on the read path reproduces the tampering message, and replacing the host check with a dot count lets the userinfo URL launch.

**Summary generated by LLM agent** Two holes at either end of the two-label rule this release adds. The first is the most serious defect the review has turned up: it makes every launch fail for an operator whose only action was to upgrade. ## A pre-existing approval now reads as tampering The rule was applied where approvals are **read**. `approved_entries` validates every entry on every read, and `bin/dejima` assigns from it under `set -euo pipefail` about 1200 lines before the launch check. So a single-label domain already in the store fails there: ``` store contains "loki" before: exit=0, launch proceeds after: corrupt approval entry in .../allowed-domains: loki exit=1, docker never invoked ``` Three things make this worse than an ordinary refusal. The global store is read on **every** launch, so this is not one project failing - it is every project, until the file is edited by hand. `dejima-allow` has no remove verb, so that is the only recovery. The message is `corrupt approval entry`, and `approved_entries`' own comment says such an entry "means a bug or tampering, not user error". It would be saying that about the operator's own file, on every launch, for a value that was valid when they approved it. The last release accepted single-label domains - a commit in this very release said so explicitly while adding support for them, before that support was reverted - so this is the store an upgrading operator has. And the launch-time message written for exactly this case is unreachable behind it. The careful error naming the key and explaining the rule is dead code; what an operator actually meets is the tampering message. **Granting is where a tightened rule belongs.** Reading keeps the shape check it had, which still refuses genuine corruption - a malformed entry like `a..b.com` fails the read as before. `approve_entry` refuses a single-label domain when it is granted, with a message that says why, and `dejima-project` classifies one in a project's declaration as rejected. ## The launch check counted dots, and both slips it was written for have one Its stated purpose was to catch a host derivation that went wrong, on the grounds that a `sed` over a URL can leave something nobody typed. It only caught that when the residue happened to be dotless: ``` https://user@host.example/ -> user@host.example was accepted http://loki./ -> loki. was accepted http://user:pw@llm.local/ -> user was refused ``` The first two reach the container and are refused there instead, which is the failure this check exists to move. It now asks the same question of the value that the policy will ask - a host name, of at least two labels - rather than a cheaper one that happens to agree in one case out of three. Its message changed shape with that, so the assertion that named the old wording now names the rule instead. ## What was checked and found sound The same pass drove the check for over-refusal and found none: IPv4 addresses, an IPv4 in `internal_domains`, punycode, uppercase, empty and unset values, and odd list whitespace all pass, and everything it refuses that another layer accepts was already refused at container start. Placement is right - `--help`, `--print-config` and `--print-briefing` all exit before it, so a diagnostic still works when a launch does not. All five sources are final at the point it runs, and none is modified afterwards. ## Testing Full confined gate, all ten suites, green. Both fixes were run against their own removal: restoring the narrow validator on the read path reproduces the tampering message, and replacing the host check with a dot count lets the userinfo URL launch.

dejima-agent added 1 commit 2026-09-14 22:41:59 -07:00

Stop a pre-existing approval reading as tampering, and check the derived host properly ... 39b7c9888d

Two holes at either end of the two-label rule.

The rule was applied where approvals are READ. approved_entries validates
every entry on every read and bin/dejima assigns from it under set -e, so a
single-label domain already in the store failed the read - in every project,
on every launch, with the store's "corrupt approval entry" message, which
means tampering. The last release accepted those domains, so that store is
the one an upgrading operator has, and their only mistake was upgrading.
`dejima-allow` has no remove verb, so the recovery was hand-editing the file
the message had just called corrupt. The launch-time message written for this
case was unreachable behind it.

Granting is where a tightened rule belongs. Reading keeps the shape check it
had, which still refuses real corruption.

The launch check counted dots, which only catches a derivation that failed in
a way that happens to leave a bare label. `https://user@host.example/` gives
`user@host.example` and `http://loki./` gives `loki.` - both have a dot,
neither is a host name, and both were the sed slips the check was written
for. It now asks the same question of the value that the policy will ask.

Its message changed shape with it, so the assertion naming the old wording
moved to the rule rather than the phrasing.

dejima-agent referenced this pull request 2026-09-14 23:57:32 -07:00

The privileged job trips GitHub's secondary rate limit on /meta #357

branderson approved these changes 2026-09-15 19:46:21 -07:00

branderson merged commit 26ee847e35 into development 2026-09-15 19:46:39 -07:00

dejima-agent referenced this pull request 2026-09-16 00:00:36 -07:00

Release: the egress filter and the container image, both rewritten clean-room #347

dejima-agent referenced this pull request 2026-09-16 00:23:52 -07:00

Finish the release's documentation review, and two things it found in code #359

Sign in to join this conversation.

Reviewers

No Reviewers

branderson

Labels

Clear labels

Compat/Breaking

Breaking change that won't be backward compatible

Kind/Bug

Something is not working

Kind/Documentation

Documentation changes

Kind/Enhancement

Improve existing functionality

Kind/Feature

New functionality

Kind/Security

This is security issue

Kind/Testing

Issue or pull request related to testing

Priority

Critical

1

The priority is critical

Priority

High

2

The priority is high

Priority

Low

4

The priority is low

Priority

Medium

3

The priority is medium

Reviewed

Confirmed

1

Issue has been confirmed

Reviewed

Duplicate

2

This issue or pull request already exists

Reviewed

Invalid

3

Invalid issue

Reviewed

Won't Fix

3

This issue won't be fixed

Status

Abandoned

3

Somebody has started to work on this but abandoned work

Status

Blocked

1

Something is blocking this issue or pull request

Status

Need More Info

2

Feedback is required to reproduce issue or to continue work

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

branderson dejima-agent

No Assignees

2 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: dejima/dejima#355