fix(cli): refuse an output destination that aliases the input by dmihalcik-virtru · Pull Request #4086 · opentdf/platform

dmihalcik-virtru · 2026-09-21T14:23:47Z

Follow-up to #3939, which has since merged. Rebased onto main.
Addresses the two open CodeRabbit threads on that PR rather than amending it.

Proposed Changes

An output destination that aliases the input. A destination a rename cannot
stand in for — a symlink, a device, a fifo — is written through directly, which
means os.OpenFile with O_TRUNC. Both encrypt and decrypt open their
input first and resolve the destination second, so -o at a symlink resolving
to the input truncated the input out from under the already-open descriptor.
The command then read an empty file, failed, and the source was gone:

$ ln -s input.tdf out.link
$ otdfctl decrypt input.tdf -o out.link     # before this change
# input.tdf is now 0 bytes, and the decrypt fails on it

NewOutputFile now takes the caller's already-open input and refuses a
write-through destination that os.SameFile says is that input. The check
stats through the link, since that is what the open does; a dangling link
resolves to nothing and is not an alias.

The guard is deliberately confined to the write-through path. An ordinary
destination naming the input goes to a temp sibling and is renamed over it only
after Decrypt has returned, so nothing is truncated and in-place decryption
keeps working — that is a reasonable thing to ask for, not a footgun.

Passing the input is a required parameter rather than an option. The whole
failure mode is a caller not thinking about it, so the signature makes that
impossible to skip; nil is available for a caller with no input to protect.

Temporary filenames no longer carry the destination's basename. A component
name is capped at 255 bytes on most filesystems, and `"." + basename + ".tmp-"

16 hexadded 22 bytes to it — so a destination whose own name was valid could fail the open before a single byte had been read. The prefix is now a fixed.otdfctl.tmp-`. The random suffix is what distinguishes concurrent writers; the
prefix only has to be recognizable enough to sweep up after a crash.

Both fixes land in streamio, so both reach encrypt and decrypt. The temp
prefix was not introduced by #3939 — it came in with #3937 — but it lives in the
same function.

Checklist

I have added or updated unit tests
I have added or updated integration tests (if appropriate)
I have added or updated documentation

Testing Instructions

cd otdfctl && go test ./... -race

New unit tests, each verified to fail against the unfixed code:

TestOutputFileRejectsSymlinkToInput — the destructive case, and that the
input is still readable afterwards.
TestOutputFileWritesThroughSymlinkToOtherFile,
TestOutputFileWritesThroughDanglingSymlink — the guard stays narrow enough
to leave write-through working.
TestOutputFileAllowsRegularDestinationNamingInput — in-place decrypt is not
rejected.
TestOutputFileAcceptsMaximumLengthDestinationName — a 255-byte destination
name opens. Probes the filesystem first and skips where a 255-byte component
is not allowed, so it reports the temp-name bug and nothing else.

e2e, against a running platform:

cd otdfctl && bats --tap e2e --filter-tags unattributed_encrypt

Two cases added to streaming.bats (12 → 14), one per command, asserting the
failure and that the input survives it. The two assert_no_leftovers globs in
that file are updated for the new temp prefix.

coderabbitai · 2026-09-21T14:23:55Z

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

github-actions · 2026-09-21T14:33:42Z

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric	Value
Approved Decision Requests	1000
Denied Decision Requests	0
Total Time	241.367504ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric	Value
Approved Decision Requests	1000
Denied Decision Requests	0
Total Time	152.725556ms

Benchmark Statistics

Name	№ Requests	Avg Duration	Min Duration	Max Duration

Bulk Benchmark Results

Metric	Value
Total Decrypts	100
Successful Decrypts	100
Failed Decrypts	0
Total Time	428.478118ms
Throughput	233.38 requests/second

TDF3 Benchmark Results:

Metric	Value
Total Requests	5000
Successful Requests	5000
Failed Requests	0
Concurrent Requests	50
Total Time	59.4800161s
Average Latency	593.337701ms
Throughput	84.06 requests/second

github-actions · 2026-09-21T14:37:50Z

X-Test Results

✅ js@main-v0.9.0
✅ go@pull-4086-main
✅ java@main-main
✅ java@main-pull-4086
govulncheck-failure-1
cukes-report
govulncheck-failure-8
✅ go@main-pull-4086
✅ go@main-v0.9.0
✅ go@pull-4086-v0.9.0
✅ go@main-main
opentdf~~platform~~TWGRNG.dockerbuild
✅ js@main-main
govulncheck-failure-3
✅ js@main-pull-4086
✅ go@pull-4086-pull-4086
✅ java@main-v0.9.0

github-actions · 2026-09-21T14:43:48Z

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric	Value
Approved Decision Requests	1000
Denied Decision Requests	0
Total Time	143.031212ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric	Value
Approved Decision Requests	1000
Denied Decision Requests	0
Total Time	78.863627ms

Benchmark Statistics

Name	№ Requests	Avg Duration	Min Duration	Max Duration

Bulk Benchmark Results

Metric	Value
Total Decrypts	100
Successful Decrypts	100
Failed Decrypts	0
Total Time	281.250143ms
Throughput	355.56 requests/second

TDF3 Benchmark Results:

Metric	Value
Total Requests	5000
Successful Requests	5000
Failed Requests	0
Concurrent Requests	50
Total Time	35.886989536s
Average Latency	358.124804ms
Throughput	139.33 requests/second

github-actions · 2026-09-21T14:53:30Z

X-Test Results

✅ java@main-main
✅ js@main-pull-4086
✅ go@pull-4086-v0.9.0
✅ go@pull-4086-pull-4086
✅ go@main-v0.9.0
✅ go@main-main
✅ js@main-v0.9.0
✅ java@main-pull-4086
✅ go@pull-4086-main
govulncheck-failure-1
govulncheck-failure-3
✅ java@main-v0.9.0
govulncheck-failure-8
✅ js@main-main
✅ go@main-pull-4086
cukes-report
opentdf~~platform~~CFUWD2.dockerbuild

dmihalcik-virtru · 2026-09-21T15:45:28Z

Considering pulling this as it has a breaking change to the streamio API

jakedoublev · 2026-09-21T16:58:08Z

@dmihalcik-virtru I agree that we can probably punt on this because if you're outputting to the same file you're decrypting, that seems like something that could pretty obviously break.

A write-through destination is opened with O_TRUNC before the payload is read, so pointing -o at a symlink resolving to the input truncated the input out from under the already-open descriptor: the command then read an empty file, failed, and the source was gone. Both encrypt and decrypt open their input first and resolve the destination second, so both walked into it. NewOutputFile now takes the input file and refuses a write-through destination that os.SameFile says is that input. An ordinary destination naming the input is left alone -- temp-and-rename never truncates the source, and in-place decrypt is a deliberate thing to ask for. Also drops the destination basename from the temporary filename. A valid 255-byte destination name produced an overlong temporary component, and NewOutputFile failed before any payload was read. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>

github-actions · 2026-09-21T21:39:04Z

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric	Value
Approved Decision Requests	1000
Denied Decision Requests	0
Total Time	176.553909ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric	Value
Approved Decision Requests	1000
Denied Decision Requests	0
Total Time	94.980321ms

Benchmark Statistics

Name	№ Requests	Avg Duration	Min Duration	Max Duration

Bulk Benchmark Results

Metric	Value
Total Decrypts	100
Successful Decrypts	100
Failed Decrypts	0
Total Time	355.391611ms
Throughput	281.38 requests/second

TDF3 Benchmark Results:

Metric	Value
Total Requests	5000
Successful Requests	5000
Failed Requests	0
Concurrent Requests	50
Total Time	43.341250169s
Average Latency	432.365339ms
Throughput	115.36 requests/second

github-actions · 2026-09-21T21:41:09Z

⚠️ Govulncheck found vulnerabilities ⚠️

The following modules have known vulnerabilities:

otdfctl
service
tests-bdd

See the workflow run for details.

github-actions · 2026-09-21T21:49:22Z

X-Test Results

govulncheck-failure-1
✅ go@main-pull-4086
✅ java@main-main
✅ go@pull-4086-v0.9.0
✅ go@pull-4086-pull-4086
✅ java@main-v0.9.0
govulncheck-failure-8
✅ js@main-v0.9.0
✅ go@main-v0.9.0
opentdf~~platform~~WUYAYN.dockerbuild
✅ go@pull-4086-main
govulncheck-failure-3
✅ js@main-pull-4086
bats-test-results
cukes-report
✅ java@main-pull-4086
✅ go@main-main
✅ js@main-main

dmihalcik-virtru requested a review from a team as a code owner September 21, 2026 14:23

github-actions Bot added the size/s label Sep 21, 2026

Base automatically changed from dspx-2604-10-stream-decrypt to main September 21, 2026 14:34

dmihalcik-virtru force-pushed the dspx-2604-10a-output-alias-guard branch from 146c56b to 229812f Compare September 21, 2026 14:38

dmihalcik-virtru marked this pull request as draft September 21, 2026 15:45

dmihalcik-virtru force-pushed the dspx-2604-10a-output-alias-guard branch from 229812f to 437649c Compare September 21, 2026 21:34

Conversation

dmihalcik-virtru commented Sep 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Checklist

Testing Instructions

Uh oh!

coderabbitai Bot commented Sep 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Draft PR not reviewed

Uh oh!

github-actions Bot commented Sep 21, 2026

Benchmark authorization.GetDecisions Results:

Benchmark authorization.v2.GetMultiResourceDecision Results:

Benchmark Statistics

Bulk Benchmark Results

TDF3 Benchmark Results:

Uh oh!

github-actions Bot commented Sep 21, 2026

X-Test Results

Uh oh!

github-actions Bot commented Sep 21, 2026

Benchmark authorization.GetDecisions Results:

Benchmark authorization.v2.GetMultiResourceDecision Results:

Benchmark Statistics

Bulk Benchmark Results

TDF3 Benchmark Results:

Uh oh!

github-actions Bot commented Sep 21, 2026

X-Test Results

Uh oh!

dmihalcik-virtru commented Sep 21, 2026

Uh oh!

jakedoublev commented Sep 21, 2026

Uh oh!

github-actions Bot commented Sep 21, 2026

Benchmark authorization.GetDecisions Results:

Benchmark authorization.v2.GetMultiResourceDecision Results:

Benchmark Statistics

Bulk Benchmark Results

TDF3 Benchmark Results:

Uh oh!

github-actions Bot commented Sep 21, 2026

⚠️ Govulncheck found vulnerabilities ⚠️

Uh oh!

github-actions Bot commented Sep 21, 2026

X-Test Results

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dmihalcik-virtru commented Sep 21, 2026 •

edited

Loading

coderabbitai Bot commented Sep 21, 2026 •

edited

Loading