fix: one wording for a rejected password on every screen (closes #172)
All checks were successful
check / check (push) Successful in 29s
All checks were successful
check / check (push) Successful in 29s
The send confirmation and the delete-wallet confirmation rendered
"Wrong password." — a fragment, which README Language & Labeling and
RULES.md:120 both forbid — while the two reveal screens said "That
password is not correct." and the two dApp approval paths said "That
password is incorrect." Three wordings for one condition, on screens a
user can reach minutes apart.
All six decryptWithPassword call sites, across the five views that own
one, now show the wording the approval paths introduced:
That password is incorrect. Please try again.
Strings only. Nothing about how a wrong password is handled changes: it
still fails closed on every screen, and the approval paths' settlement,
claim/release interlock and retry behaviour are untouched.
The new test scans the source for the call sites rather than driving
each view, because the invariant is about the set: a seventh call site
has to join it, and a per-view test cannot notice a screen nobody wrote
one for. It asserts per call site, not per file — each decrypt is read
back to its own catch handler and the prose that handler shows must be
the canonical sentence and nothing else. approval.js decrypts twice and
is where the divergence came from, so a per-file check that only asks
whether the sentence appears somewhere in the file passes while one of
those two says something novel. Exact equality catches a new wording,
not only a known-superseded one.
This commit is contained in:
213
tests/passwordMessages.test.js
Normal file
213
tests/passwordMessages.test.js
Normal file
@@ -0,0 +1,213 @@
|
||||
// One wording for one condition (issue #172).
|
||||
//
|
||||
// Every screen that asks for the password decrypts the vault itself, and
|
||||
// each one used to write its own sentence for the same failure: the send
|
||||
// confirmation and the delete-wallet confirmation said "Wrong password."
|
||||
// (a fragment, which RULES.md Language & Labeling forbids), the reveal
|
||||
// screens said "That password is not correct.", and the two dApp approval
|
||||
// paths said "That password is incorrect." A user hitting two of those
|
||||
// minutes apart had no way to tell whether the wallet meant the same
|
||||
// thing.
|
||||
//
|
||||
// This scans the source rather than driving six views, because the
|
||||
// invariant is about the set of call sites and not about any one of them:
|
||||
// a seventh screen that decrypts the vault has to join the set, and a
|
||||
// DOM test per view cannot notice one that was never written.
|
||||
//
|
||||
// The assertions are per CALL SITE, not per file. approval.js decrypts in
|
||||
// two places and is where the divergence came from; a per-file check that
|
||||
// only asks whether the canonical sentence appears somewhere in the file
|
||||
// passes while one of those two says something else entirely. So each
|
||||
// call site is read back to its own catch handler and the prose that
|
||||
// handler shows the user must be the canonical sentence and nothing else
|
||||
// — which fails on a novel wording, not only on a known-superseded one.
|
||||
|
||||
const fs = require("fs");
|
||||
const path = require("path");
|
||||
|
||||
const SRC = path.join(__dirname, "..", "src");
|
||||
|
||||
const CANONICAL = "That password is incorrect. Please try again.";
|
||||
|
||||
// Wordings this repo has actually shipped for the same condition. This is
|
||||
// a secondary, whole-file sweep for stragglers outside a decrypt handler;
|
||||
// divergence at a call site is caught by the exact-match assertion, which
|
||||
// needs no list of phrasings to guess at.
|
||||
const SUPERSEDED = [
|
||||
"Wrong password.",
|
||||
"That password is not correct. Please try again.",
|
||||
];
|
||||
|
||||
function jsFilesUnder(dir) {
|
||||
return fs.readdirSync(dir, { withFileTypes: true }).flatMap((entry) => {
|
||||
const full = path.join(dir, entry.name);
|
||||
if (entry.isDirectory()) return jsFilesUnder(full);
|
||||
return entry.name.endsWith(".js") ? [full] : [];
|
||||
});
|
||||
}
|
||||
|
||||
// Blank out the interior of every comment and string literal, keeping the
|
||||
// offsets and line breaks, so braces can be counted without a quote or a
|
||||
// commented-out block throwing the count off. The literals are returned
|
||||
// alongside with the offset of their opening quote, which is how a
|
||||
// message is later attributed to the handler it sits in.
|
||||
function scan(source) {
|
||||
const masked = source.split("");
|
||||
const strings = [];
|
||||
const blank = (from, to) => {
|
||||
for (let k = from; k < to; k++) if (masked[k] !== "\n") masked[k] = " ";
|
||||
};
|
||||
let i = 0;
|
||||
while (i < source.length) {
|
||||
const two = source.slice(i, i + 2);
|
||||
if (two === "//") {
|
||||
const nl = source.indexOf("\n", i);
|
||||
const stop = nl === -1 ? source.length : nl;
|
||||
blank(i, stop);
|
||||
i = stop;
|
||||
} else if (two === "/*") {
|
||||
const close = source.indexOf("*/", i + 2);
|
||||
const stop = close === -1 ? source.length : close + 2;
|
||||
blank(i, stop);
|
||||
i = stop;
|
||||
} else if (
|
||||
source[i] === '"' ||
|
||||
source[i] === "'" ||
|
||||
source[i] === "`"
|
||||
) {
|
||||
const quote = source[i];
|
||||
let j = i + 1;
|
||||
let value = "";
|
||||
while (j < source.length && source[j] !== quote) {
|
||||
if (source[j] === "\\") {
|
||||
value += source[j + 1];
|
||||
j += 2;
|
||||
continue;
|
||||
}
|
||||
value += source[j];
|
||||
j += 1;
|
||||
}
|
||||
blank(i + 1, j);
|
||||
strings.push({ offset: i, value });
|
||||
i = j + 1;
|
||||
} else {
|
||||
i += 1;
|
||||
}
|
||||
}
|
||||
return { masked: masked.join(""), strings };
|
||||
}
|
||||
|
||||
// Offset of the `{` that opens the block containing `at`, or -1.
|
||||
function enclosingBlockStart(masked, at) {
|
||||
let depth = 0;
|
||||
for (let i = at; i >= 0; i--) {
|
||||
if (masked[i] === "}") depth += 1;
|
||||
else if (masked[i] === "{") {
|
||||
if (depth === 0) return i;
|
||||
depth -= 1;
|
||||
}
|
||||
}
|
||||
return -1;
|
||||
}
|
||||
|
||||
// Offset just past the `}` matching the `{` at `open`.
|
||||
function blockEnd(masked, open) {
|
||||
let depth = 0;
|
||||
for (let i = open; i < masked.length; i++) {
|
||||
if (masked[i] === "{") depth += 1;
|
||||
else if (masked[i] === "}") {
|
||||
depth -= 1;
|
||||
if (depth === 0) return i + 1;
|
||||
}
|
||||
}
|
||||
throw new Error("unterminated block");
|
||||
}
|
||||
|
||||
// The catch handler guarding a given decryptWithPassword call: walk out to
|
||||
// the try block the call sits in, then take the catch that follows it.
|
||||
function handlerSpan(masked, callOffset, label) {
|
||||
const tryOpen = enclosingBlockStart(masked, callOffset);
|
||||
if (tryOpen === -1 || !/\btry\s*$/.test(masked.slice(0, tryOpen)))
|
||||
throw new Error(`${label}: the decrypt is not inside a try block`);
|
||||
const rest = masked.slice(blockEnd(masked, tryOpen));
|
||||
const catchMatch = /^\s*catch\s*(\([^)]*\)\s*)?\{/.exec(rest);
|
||||
if (!catchMatch)
|
||||
throw new Error(`${label}: the decrypt's try block has no catch`);
|
||||
const catchOpen = blockEnd(masked, tryOpen) + catchMatch[0].length - 1;
|
||||
return [catchOpen, blockEnd(masked, catchOpen)];
|
||||
}
|
||||
|
||||
// The prose the handler puts in front of the user. Element ids, class
|
||||
// names and visibility keywords are single words; a sentence has a space
|
||||
// in it, and that is the whole distinction needed here.
|
||||
function handlerMessages(file, callOffset, label) {
|
||||
const { masked, strings } = scan(fs.readFileSync(file, "utf8"));
|
||||
const [from, to] = handlerSpan(masked, callOffset, label);
|
||||
return strings
|
||||
.filter((s) => s.offset >= from && s.offset < to)
|
||||
.map((s) => s.value)
|
||||
.filter((v) => v.includes(" "));
|
||||
}
|
||||
|
||||
// The call sites are found, not listed: the file layout moves (the private
|
||||
// key export was in addressDetail.js when #172 was filed and is its own
|
||||
// view now), and a hardcoded list would quietly stop covering a screen it
|
||||
// no longer names.
|
||||
function callSites() {
|
||||
const sites = [];
|
||||
for (const file of jsFilesUnder(SRC)) {
|
||||
if (file === path.join(SRC, "shared", "vault.js")) continue;
|
||||
const { masked } = scan(fs.readFileSync(file, "utf8"));
|
||||
const rel = path.relative(SRC, file).split(path.sep).join("/");
|
||||
let n = 0;
|
||||
let at = masked.indexOf("decryptWithPassword(");
|
||||
while (at !== -1) {
|
||||
n += 1;
|
||||
sites.push({ file, rel, offset: at, label: `${rel} #${n}` });
|
||||
at = masked.indexOf("decryptWithPassword(", at + 1);
|
||||
}
|
||||
}
|
||||
return sites.sort((a, b) => a.label.localeCompare(b.label));
|
||||
}
|
||||
|
||||
describe("password failure messages", () => {
|
||||
const sites = callSites();
|
||||
const files = [...new Set(sites.map((s) => s.file))].sort();
|
||||
|
||||
test("the call sites are found where they are expected", () => {
|
||||
const counts = {};
|
||||
for (const site of sites)
|
||||
counts[site.rel] = (counts[site.rel] ?? 0) + 1;
|
||||
expect(counts).toEqual({
|
||||
"popup/views/approval.js": 2,
|
||||
"popup/views/confirmTx.js": 1,
|
||||
"popup/views/deleteWallet.js": 1,
|
||||
"popup/views/exportPrivkey.js": 1,
|
||||
"popup/views/showPhrase.js": 1,
|
||||
});
|
||||
});
|
||||
|
||||
test("the canonical message is a full sentence", () => {
|
||||
expect(CANONICAL).toMatch(/^[A-Z][^]*\.$/);
|
||||
});
|
||||
|
||||
// Exact equality, per call site: a message that is merely different
|
||||
// rather than known-obsolete fails here too, which a scan for historic
|
||||
// wordings cannot do.
|
||||
test.each(sites.map((s) => [s.label, s]))(
|
||||
"%s answers a rejected password with the canonical sentence",
|
||||
(label, site) => {
|
||||
expect(handlerMessages(site.file, site.offset, label)).toEqual([
|
||||
CANONICAL,
|
||||
]);
|
||||
},
|
||||
);
|
||||
|
||||
test.each(files.map((f) => [path.relative(SRC, f), f]))(
|
||||
"%s carries no superseded wording",
|
||||
(_rel, file) => {
|
||||
const source = fs.readFileSync(file, "utf8");
|
||||
for (const old of SUPERSEDED) expect(source).not.toContain(old);
|
||||
},
|
||||
);
|
||||
});
|
||||
Reference in New Issue
Block a user