fix: correct reprint threshold display and de-duplicate the formula (#124)
Some checks failed
CodeQL / Analyze (javascript) (push) Has been cancelled
End to end test / e2e-test (push) Has been cancelled
Trivy scan / Analyze (push) Has been cancelled
Yarn tests / yarn (push) Has been cancelled

* fix: correct reprint threshold display and de-duplicate the formula (F5)

Print.vue recomputed the recovery threshold as floor(total/2)+2 and stored
the *total* in a field misleadingly named `requiredShards`. The reprint
sheets therefore overstated how many more QR codes are needed (e.g. total=5
showed "need 4" instead of the correct 3) — misleading during recovery,
though the QR payloads themselves were always untouched.

Root cause: the threshold policy was duplicated. Share.vue computes
floor(total/2)+1; Print.vue re-implemented it and drifted. Extract a single
`defaultThreshold()` helper (src/util/shards.ts) and use it in both, so they
can never disagree again. Rename Print.vue's field to `totalShards` to match
what it actually holds.

Add unit tests for the helper, including the exact F5 case (total=5 -> 3) and
a strict-majority invariant across the whole 3..255 UI range.

* fix: reject fractional shard counts, clamp the remaining-code count

Review follow-ups on the reprint view.

`<input type="number">` only constrains the spinner, not the value: typing
"3.5" (or clearing the box, which `v-model.number` leaves as "") sailed
through the old `>= 3 && <= 255` check and reached defaultThreshold(),
producing fractional totals and remaining counts. Add `step="1"` for the
spinner and gate on a shared `isValidShardCount()` predicate that also
requires a whole number, so the input and the threshold policy keep agreeing
about what a shard count is.

`needMoreShards` compared with `!==`, so scanning more codes than announced
kept the scanner open and drove `remainingCodes` negative. Compare with `<`
and clamp the remainder at 0.

`threshold` returned 0 for "nothing entered yet", a valid-looking value that
could reach ShardInfo's required-shards prop. Return `undefined` instead and
guard the consuming block on it, making the not-yet-entered state explicit.

The `threshold` and validation paths are covered by shards.spec.ts; the
scanner-overshoot clamp is not, as the repo has no component-test harness yet.

* fix: apply the same shard-count validation to the generator

Share.vue's shard-count input had the defect Copilot flagged on Print.vue's:
no `step`, no integer check, and `totalShards` fed straight into
crypto.share() — a fractional count surfaced as an opaque secrets.js error
routed through the generic error hub, and an emptied box passed "" through.

Reuse `isValidShardCount()` and gate the generate button on it, matching the
existing `secretTooLong` pattern (disabled button plus an inline error span),
so both shard-count inputs now agree on what a shard count is.

* fix: stop rendering a threshold derived from an invalid shard count

Gating the generate button left the sentence above it still interpolating
defaultThreshold(totalShards) for values that had just been rejected: an
emptied field coerces to 0 and renders "Will require any 1 shards", and 3.5
renders 2. Give Share.vue's `requiredShards` the same contract as Print.vue's
`threshold` — `undefined` unless the count is usable — show an em dash in the
sentence, and guard both the generated-shards block and crypto.share() on it.

Also strengthen the threshold invariant test. Asserting only
`defaultThreshold(n) > n / 2` does not pin the policy: floor(n/2)+2, the very
formula this branch removed, satisfies it for every even n. Assert the
smallest strict majority instead, which floor(n/2)+1 alone satisfies.
This commit is contained in:
Kirill Pimenov 2026-08-04 16:10:30 +02:00 committed by GitHub
parent 11f18074f4
commit 0c77cc4534
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 115 additions and 16 deletions

16
src/util/shards.ts Normal file
View File

@ -0,0 +1,16 @@
// Banana Split's fixed threshold policy: reconstruction requires a majority of
// the shards, i.e. floor(total / 2) + 1. This lives in one place so the
// generator (Share.vue) and the reprint view (Print.vue) can never disagree —
// they did once: Print.vue re-implemented the formula as floor(total/2)+2 and
// displayed the wrong "you need N more" count on reprints (finding F5).
export function defaultThreshold(totalShards: number): number {
return Math.floor(totalShards / 2) + 1;
}
// Mirrors the min/max on the shard-count inputs. `v-model.number` on an
// `<input type="number">` hands us whatever the field holds, which includes ""
// for an empty box and fractions like 3.5 — neither is a shard count, so the
// range check alone is not enough.
export function isValidShardCount(value: unknown): value is number {
return typeof value === "number" && Number.isInteger(value) && value >= 3 && value <= 255;
}

View File

@ -11,10 +11,11 @@
<br>
<input
id="totalShards"
v-model.number="requiredShards"
v-model.number="totalShards"
type="number"
min="3"
max="255"
step="1"
/>
</p>
<button id="generateBtn" class="button-card" @click="handleShardsInput">
@ -28,7 +29,7 @@
<div v-if="numberEntered && needMoreShards">
<qrcode-stream @decode="onDecode" />
</div>
<div v-else-if="numberEntered">
<div v-else-if="numberEntered && threshold !== undefined">
<button id="printBtn" class="button-card" @click="print">
Print us!
</button>
@ -36,7 +37,7 @@
v-for="code in qrCodes"
:key="code"
:shard="code"
:required-shards="parseInt(requiredShards/2)+2"
:required-shards="threshold"
:title="title"
/>
</div>
@ -64,6 +65,7 @@
<script lang="ts">
import crypto, { Shard } from "../util/crypto";
import { defaultThreshold, isValidShardCount } from "../util/shards";
import ShardInfo from "../components/ShardInfo.vue";
import Vue from "vue";
@ -73,7 +75,7 @@ type PrintData = {
nonce: string;
shards: Shard[];
qrCodes: Set<string>;
requiredShards?: number;
totalShards?: number;
numberEntered: boolean;
PLACEHOLDER_QR_DATA: string;
};
@ -87,21 +89,28 @@ export default Vue.extend({
nonce: "",
shards: [],
qrCodes: new Set(),
requiredShards: undefined,
totalShards: undefined,
numberEntered: false,
PLACEHOLDER_QR_DATA: ""
};
},
computed: {
needMoreShards(): boolean {
return this.requiredShards !== undefined && this.shards.length !== this.requiredShards;
// Strictly "fewer than": scanning more codes than announced must not keep
// the scanner open (and must not make `remainingCodes` go negative).
return this.totalShards !== undefined && this.shards.length < this.totalShards;
},
remainingCodes(): number {
if (!this.requiredShards) {
if (!this.totalShards) {
return 0;
} else {
return this.requiredShards - this.shards.length;
return Math.max(0, this.totalShards - this.shards.length);
}
},
// `undefined` until a valid count has been entered, so "not asked yet" stays
// distinguishable from a real threshold; the template guards on it.
threshold(): number | undefined {
return isValidShardCount(this.totalShards) ? defaultThreshold(this.totalShards) : undefined;
}
},
mounted: function() {
@ -144,10 +153,10 @@ export default Vue.extend({
window.print();
},
handleShardsInput: function() {
if (this.requiredShards && this.requiredShards >= 3 && this.requiredShards <= 255) {
if (isValidShardCount(this.totalShards)) {
this.numberEntered = true;
} else {
this.$eventHub.$emit("showError", "Please enter a valid number of shards between 3 and 255.");
this.$eventHub.$emit("showError", "Please enter a whole number of shards between 3 and 255.");
}
},
toggleMode: function() {

View File

@ -31,7 +31,7 @@
<p>
<label>3. Shards</label>
<br />
Will require any {{ requiredShards }} shards out of
Will require any {{ requiredShardsLabel }} shards out of
<input
id="totalShards"
v-model.number="totalShards"
@ -39,13 +39,18 @@
type="number"
min="3"
max="255"
step="1"
/>
to reconstruct
<br />
<span v-if="!shardCountValid" class="error-text">
Enter a whole number of shards between 3 and 255
</span>
</p>
<button
id="generateBtn"
class="button-card"
:disabled="secretTooLong"
:disabled="secretTooLong || !shardCountValid"
:hidden="encryptionMode"
v-on:click="toggleMode"
>
@ -62,7 +67,7 @@
</button>
</div>
<div v-if="encryptionMode">
<div v-if="encryptionMode && requiredShards !== undefined">
<div class="card" framed="true" transparent="true">
<label>4. Your passphrase for the recovery is:</label>
<div class="flex justify-between align-center">
@ -91,6 +96,7 @@
<script lang="ts">
import passPhrase from "../util/passPhrase";
import crypto from "../util/crypto";
import { defaultThreshold, isValidShardCount } from "../util/shards";
import ShardInfo from "../components/ShardInfo.vue";
import CanvasText from "../components/CanvasText.vue";
@ -120,12 +126,23 @@ export default Vue.extend({
secretTooLong(): boolean {
return this.secret.length > 1024;
},
requiredShards(): number {
return Math.floor(this.totalShards / 2) + 1;
// Gates generation the same way `secretTooLong` does: a fractional or empty
// count would otherwise reach crypto.share() and fail deep inside secrets.js.
shardCountValid(): boolean {
return isValidShardCount(this.totalShards);
},
// Same contract as Print.vue's `threshold`: `undefined` while the count is
// not usable, so a coerced value ("" divides to 0, giving a bogus 1) can
// neither be printed nor reach ShardInfo's required Number prop.
requiredShards(): number | undefined {
return isValidShardCount(this.totalShards) ? defaultThreshold(this.totalShards) : undefined;
},
requiredShardsLabel(): string {
return this.requiredShards === undefined ? "—" : String(this.requiredShards);
},
shards(): string[] {
this.$eventHub.$emit("clearAlerts");
if (!this.encryptionMode) {
if (!this.encryptionMode || this.requiredShards === undefined) {
return [];
}
try {

57
tests/unit/shards.spec.ts Normal file
View File

@ -0,0 +1,57 @@
import { defaultThreshold, isValidShardCount } from "../../src/util/shards";
describe("defaultThreshold", () => {
// The bug fixed in F5 was Print.vue computing floor(total/2)+2 instead of
// floor(total/2)+1 — e.g. total=5 showed "need 4" instead of the correct 3.
test.each([
[3, 2],
[4, 3],
[5, 3], // the exact case from finding F5
[6, 4],
[7, 4],
[10, 6],
[19, 10],
[255, 128]
])("threshold for %i total shards is %i", (total, expected) => {
expect(defaultThreshold(total)).toBe(expected);
});
// A lower bound alone is too weak to pin the policy: floor(8/2)+2 = 6 is also
// "more than half of 8". Assert the threshold is the *smallest* strict
// majority, which is the property that makes floor(n/2)+1 the only answer.
test("is the smallest strict majority across the whole UI range", () => {
for (let n = 3; n <= 255; n++) {
const threshold = defaultThreshold(n);
expect(threshold).toBeGreaterThan(n / 2);
expect(threshold - 1).toBeLessThanOrEqual(n / 2);
}
});
});
describe("isValidShardCount", () => {
test("accepts every whole number the UI offers", () => {
for (let n = 3; n <= 255; n++) {
expect(isValidShardCount(n)).toBe(true);
}
});
// Annotated as unknown[][] on purpose: these are the values the input can
// actually hand us, not just out-of-range numbers.
const rejected: unknown[][] = [
[2, "below the minimum"],
[256, "above the maximum"],
[0, "zero"],
[-5, "negative"],
[3.5, "fractional — type=number accepts it, step=1 only nudges the spinner"],
[Number.NaN, "NaN"],
[Number.POSITIVE_INFINITY, "infinite"],
["", "an emptied input, which v-model.number leaves as a string"],
["5", "a numeric string"],
[undefined, "the initial data value"],
[null, "null"]
];
test.each(rejected)("rejects %p (%s)", value => {
expect(isValidShardCount(value)).toBe(false);
});
});