From aab6ccf7fc8e6115fabd516b24211d8f5531ca53 Mon Sep 17 00:00:00 2001 From: Thomas Durieux <5577568+tdurieux@users.noreply.github.com> Date: Wed, 22 Jul 2026 04:28:01 +0200 Subject: [PATCH] fix: harden anonymization privacy paths (#754) --- src/core/AnonymizedFile.ts | 111 +++++++++++++++-------- src/core/Repository.ts | 13 ++- src/core/anonymize-utils.ts | 105 +++++++++++++++++++-- test/anonymize-utils.test.js | 37 ++++++++ test/anonymized-file-custom-path.test.js | 46 ++++++++++ 5 files changed, 262 insertions(+), 50 deletions(-) create mode 100644 test/anonymized-file-custom-path.test.js diff --git a/src/core/AnonymizedFile.ts b/src/core/AnonymizedFile.ts index f7136b1..b0a8239 100644 --- a/src/core/AnonymizedFile.ts +++ b/src/core/AnonymizedFile.ts @@ -7,7 +7,11 @@ import got from "got"; import Repository from "./Repository"; import { RepositoryStatus } from "./types"; import config from "../config"; -import { anonymizePath, isTextFile } from "./anonymize-utils"; +import { + anonymizePath, + hasCustomTermReplacement, + isTextFile, +} from "./anonymize-utils"; import AnonymousError from "./AnonymousError"; import { handleError } from "../server/routes/route-utils"; import FileModel from "./model/files/files.model"; @@ -17,6 +21,27 @@ import { createLogger, serializeError } from "./logger"; const logger = createLogger("anonymized-file"); +function escapeRegex(value: string): string { + return value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); +} + +function defaultMaskCandidateRegex(value: string): RegExp { + const mask = new RegExp( + `${escapeRegex(config.ANONYMIZATION_MASK)}(?:-[0-9]+)?`, + "g" + ); + let source = "^"; + let lastIndex = 0; + let match: RegExpExecArray | null; + while ((match = mask.exec(value)) !== null) { + source += escapeRegex(value.slice(lastIndex, match.index)); + source += "[^/]+"; + lastIndex = match.index + match[0].length; + } + source += escapeRegex(value.slice(lastIndex)) + "$"; + return new RegExp(source); +} + // Map a streamer error response to an AnonymousError that preserves the // upstream status and error code instead of collapsing every failure into a // generic 404. Without this, a corrupt cache, a 5xx from the streamer, an @@ -129,24 +154,33 @@ export default class AnonymizedFile { if (fileDir.endsWith("/")) fileDir = fileDir.slice(0, -1); const filename = basename(this.anonymizedPath); - if (!this.anonymizedPath.includes(config.ANONYMIZATION_MASK)) { - if (this.anonymizedPath == "") { - return { - name: "", - path: "", - repoId: this.repository.repoId, - }; - } - const query: FilterQuery = { + if (this.anonymizedPath == "") { + return { + name: "", + path: "", repoId: this.repository.repoId, - path: fileDir, }; - if (filename != "") query.name = filename; - const res = await FileModel.findOne(query); - if (res) { - this._file = res; - return res; - } + } + + // Always try the path verbatim first. Most paths contain no configured + // term, even when the repository uses custom replacements. + const exactQuery: FilterQuery = { + repoId: this.repository.repoId, + path: fileDir, + }; + if (filename != "") exactQuery.name = filename; + const exact = await FileModel.findOne(exactQuery); + if (exact) { + this._file = exact; + return exact; + } + + const terms = this.repository.options.terms || []; + const usesDefaultMask = this.anonymizedPath.includes( + config.ANONYMIZATION_MASK + ); + const usesCustomReplacement = hasCustomTermReplacement(terms); + if (!usesDefaultMask && !usesCustomReplacement) { // The stored tree can be incomplete: GitHub truncates tree listings of // very large repositories, and folders recorded in `truncatedFolders` // have entries that never made it into the database. Ask GitHub @@ -164,36 +198,37 @@ export default class AnonymizedFile { }); } - const pathQuery = fileDir - .split("/") - .map((p) => { - if (p.includes(config.ANONYMIZATION_MASK)) { - return "[^/]+"; - } - return p; - }) - .join("/"); - const nameQuery = filename.replace( - new RegExp(config.ANONYMIZATION_MASK + "(-[0-9]+)?"), - "[^/]+" - ); - - const candidates = await FileModel.find({ - repoId: this.repository.repoId, - path: new RegExp(pathQuery), - name: new RegExp(nameQuery), - }).exec(); + // Custom replacements do not carry a marker that can be reversed into a + // narrow Mongo query. Fetch the repository's paths and verify them by + // re-applying anonymization. Default XXXX-N masks retain the optimized, + // anchored query. + const candidates = usesCustomReplacement + ? await FileModel.find({ repoId: this.repository.repoId }).exec() + : await FileModel.find({ + repoId: this.repository.repoId, + path: defaultMaskCandidateRegex(fileDir), + name: defaultMaskCandidateRegex(filename), + }).exec(); for (const candidate of candidates) { const candidatePath = join(candidate.path, candidate.name); if ( - anonymizePath(candidatePath, this.repository.options.terms || []) == - this.anonymizedPath + anonymizePath(candidatePath, terms) == this.anonymizedPath ) { this._file = candidate; return candidate; } } + + // If applying the configured terms does not alter the requested path, it + // may simply be absent from a truncated tree and can be recovered as-is. + if (anonymizePath(this.anonymizedPath, terms) === this.anonymizedPath) { + const recovered = await this.recoverTruncatedFile(fileDir); + if (recovered) { + this._file = recovered; + return recovered; + } + } throw new AnonymousError("file_not_found", { object: this, httpStatus: 404, diff --git a/src/core/Repository.ts b/src/core/Repository.ts index 653f1b2..3965230 100644 --- a/src/core/Repository.ts +++ b/src/core/Repository.ts @@ -5,7 +5,11 @@ import * as sha1 from "crypto-js/sha1"; import User from "./User"; import GitHubStream from "./source/GitHubStream"; import Zip from "./source/Zip"; -import { anonymizePathCompiled, compileTerms } from "./anonymize-utils"; +import { + anonymizePathCompiled, + compileTerms, + hasCustomTermReplacement, +} from "./anonymize-utils"; import UserModel from "./model/users/users.model"; import { IAnonymizedRepositoryDocument } from "./model/anonymizedRepositories/anonymizedRepositories.types"; import { AnonymizeTransformer } from "./anonymize-utils"; @@ -141,6 +145,7 @@ export default class Repository { force: false, } ): Promise { + const terms = this._model.options.terms || []; let hasFile = await FileModel.exists({ repoId: this.repoId }).exec(); // Files created by GitHubDownload don't carry a valid 40-char GitHub // blob SHA. When the source type later switches to GitHubStream the @@ -183,7 +188,11 @@ export default class Repository { ).exec(); } } - if (opt.path?.includes(config.ANONYMIZATION_MASK)) { + if ( + opt.path && + (opt.path.includes(config.ANONYMIZATION_MASK) || + hasCustomTermReplacement(terms)) + ) { const f = new AnonymizedFile({ repository: this, anonymizedPath: opt.path, diff --git a/src/core/anonymize-utils.ts b/src/core/anonymize-utils.ts index 4115e64..4599ac4 100644 --- a/src/core/anonymize-utils.ts +++ b/src/core/anonymize-utils.ts @@ -144,7 +144,9 @@ export class AnonymizeTransformer extends Transform { // byte, isn't silently corrupted by a UTF-8 round-trip through the // StringDecoder. See discussion in #493. private pendingBytes: Buffer = Buffer.alloc(0); - private static readonly OVERLAP = 4096; + private static readonly DEFAULT_OVERLAP = 4096; + private readonly overlap: number; + private readonly bufferWholeStream: boolean; constructor( readonly opt: { @@ -160,6 +162,26 @@ export class AnonymizeTransformer extends Transform { this.nameVerdict = classifyByName(this.opt.filePath); if (this.nameVerdict !== null) this.isText = this.nameVerdict; this.anonimizer = new ContentAnonimizer(this.opt); + + const termPatterns = (opt.terms || []).map((term) => parseTermSpec(term).term); + // Streaming replacement can only safely emit a prefix when every possible + // match is shorter than the retained suffix. Regex quantifiers such as + // `*`, `+`, and `{m,n}` can exceed any fixed overlap, as can the URL/image + // removal patterns. Buffer those uncommon configurations in full (bounded + // by MAX_FILE_SIZE) so anonymization fails closed instead of leaking a + // match that straddles the streaming boundary. + this.bufferWholeStream = + opt.link === false || + opt.image === false || + termPatterns.some(termNeedsWholeStream); + const longestFixedPattern = Math.max( + 0, + ...termPatterns.map((term) => term.length), + (opt.repoName?.length || 0) + (opt.branchName?.length || 0) + 128 + ); + this.overlap = this.bufferWholeStream + ? Number.POSITIVE_INFINITY + : Math.max(AnonymizeTransformer.DEFAULT_OVERLAP, longestFixedPattern + 32); } get wasAnonimized() { @@ -174,7 +196,7 @@ export class AnonymizeTransformer extends Transform { return reencoded.length === candidate.length && reencoded.equals(candidate); } - _transform(chunk: Buffer, encoding: string, callback: () => void) { + _transform(chunk: Buffer, encoding: string, callback: (error?: Error) => void) { if (this.nameVerdict === null) { // Name didn't decide. isbinaryfile inspects the first 512 bytes for // null bytes and non-printable ratio and returns a decisive boolean. @@ -196,8 +218,19 @@ export class AnonymizeTransformer extends Transform { this.pending += this.decoder.write(chunk); this.pendingBytes = Buffer.concat([this.pendingBytes, chunk]); - if (this.pending.length > AnonymizeTransformer.OVERLAP) { - let split = this.pending.length - AnonymizeTransformer.OVERLAP; + if ( + this.bufferWholeStream && + this.pendingBytes.length > config.MAX_FILE_SIZE + ) { + return callback( + new Error( + `Text file exceeded ${config.MAX_FILE_SIZE} bytes while buffering an unbounded anonymization pattern` + ) + ); + } + + if (this.pending.length > this.overlap) { + let split = this.pending.length - this.overlap; // Avoid splitting a UTF-16 surrogate pair. const code = this.pending.charCodeAt(split); if (code >= 0xdc00 && code <= 0xdfff) { @@ -311,6 +344,45 @@ function hasCatastrophicBacktracking(src: string): boolean { return false; } +function termNeedsWholeStream(term: string): boolean { + try { + new RegExp(term, "i"); + } catch { + // Invalid regular expressions are escaped and treated literally. + return false; + } + if (hasCatastrophicBacktracking(term)) { + // Catastrophic expressions are also escaped and treated literally. + return false; + } + + let escaped = false; + let inCharacterClass = false; + for (let i = 0; i < term.length; i++) { + const char = term[i]; + if (escaped) { + escaped = false; + continue; + } + if (char === "\\") { + escaped = true; + continue; + } + if (char === "[") { + inCharacterClass = true; + continue; + } + if (char === "]") { + inCharacterClass = false; + continue; + } + if (!inCharacterClass && (char === "*" || char === "+" || char === "{")) { + return true; + } + } + return false; +} + function compileTerms(terms: string[] | undefined): CompiledTermVariant[] { if (!terms || terms.length === 0) return []; const compiled: CompiledTermVariant[] = []; @@ -379,13 +451,16 @@ export class ContentAnonimizer { ) { this.compiledTerms = compileTerms(opt.terms); if (opt.repoName && opt.branchName) { - const r = opt.repoName; - const b = opt.branchName; + const r = escapeRegex(opt.repoName); + const b = escapeRegex(opt.branchName); this.selfLinkRegexes = [ - new RegExp(`https://raw.githubusercontent.com/${r}/${b}\\b`, "gi"), - new RegExp(`https://github.com/${r}/blob/${b}\\b`, "gi"), - new RegExp(`https://github.com/${r}/tree/${b}\\b`, "gi"), - new RegExp(`https://github.com/${r}`, "gi"), + new RegExp( + `https://raw\\.githubusercontent\\.com/${r}/${b}(?=$|[/?#])`, + "gi" + ), + new RegExp(`https://github\\.com/${r}/blob/${b}(?=$|[/?#])`, "gi"), + new RegExp(`https://github\\.com/${r}/tree/${b}(?=$|[/?#])`, "gi"), + new RegExp(`https://github\\.com/${r}(?=$|[/?#])`, "gi"), ]; } } @@ -454,6 +529,12 @@ export function anonymizePath(path: string, terms: string[]) { return anonymizePathCompiled(path, compileTerms(terms)); } +export function hasCustomTermReplacement(terms: string[] | undefined): boolean { + return (terms || []).some( + (term) => parseTermSpec(term).replacement !== null + ); +} + // Variant that accepts pre-compiled term regexes — call sites that anonymize // many paths in a row (tree traversal) should compile once and reuse. export function anonymizePathCompiled( @@ -468,3 +549,7 @@ export function anonymizePathCompiled( export { compileTerms }; export type { CompiledTermVariant }; + +function escapeRegex(value: string): string { + return value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); +} diff --git a/test/anonymize-utils.test.js b/test/anonymize-utils.test.js index e195382..bcb4fc2 100644 --- a/test/anonymize-utils.test.js +++ b/test/anonymize-utils.test.js @@ -2,6 +2,10 @@ const { expect } = require("chai"); const { Transform } = require("stream"); const { StringDecoder } = require("string_decoder"); require("ts-node/register/transpile-only"); +const { + ContentAnonimizer: RealContentAnonimizer, + AnonymizeTransformer: RealAnonymizeTransformer, +} = require("../src/core/anonymize-utils"); const { withWordBoundaries, termVariants, @@ -449,6 +453,20 @@ describe("ContentAnonimizer", function () { expect(result).to.include("anonymous.4open.science/r/abc123"); }); + it("escapes regex characters in repository and branch names", function () { + const anon = new RealContentAnonimizer({ + repoName: "secret-owner/secret.repo", + branchName: "release+candidate", + repoId: "abc123", + }); + const result = anon.anonymize( + "https://raw.githubusercontent.com/secret-owner/secret.repo/release+candidate/src/index.ts" + ); + expect(result).to.equal( + "https://anonymous.4open.science/r/abc123/src/index.ts" + ); + }); + it("is case-insensitive for repo name", function () { const anon = new ContentAnonimizer({ repoName: "Owner/Repo", @@ -647,6 +665,25 @@ describe("AnonymizeTransformer (streaming)", function () { expect(result).to.equal("Created by XXXX-1 at 2025/01/01"); }); + it("does not leak matches longer than the normal streaming overlap", async function () { + const secret = "A".repeat(5000); + const transformer = new RealAnonymizeTransformer({ + filePath: "fixture.txt", + terms: ["A{5000}"], + }); + const chunks = []; + const done = new Promise((resolve, reject) => { + transformer.on("data", (chunk) => chunks.push(Buffer.from(chunk))); + transformer.on("end", () => + resolve(Buffer.concat(chunks).toString("utf8")) + ); + transformer.on("error", reject); + }); + transformer.end(Buffer.from(` ${secret} `)); + const result = await done; + expect(result).to.equal(" XXXX-1 "); + }); + // Regression for #342/#349: zip download was constructing the transformer // and then assigning opt.filePath after the fact, but isText is decided in // the constructor — so every entry was treated as binary and passed through diff --git a/test/anonymized-file-custom-path.test.js b/test/anonymized-file-custom-path.test.js new file mode 100644 index 0000000..132e790 --- /dev/null +++ b/test/anonymized-file-custom-path.test.js @@ -0,0 +1,46 @@ +const { expect } = require("chai"); +require("ts-node/register/transpile-only"); + +const AnonymizedFile = require("../src/core/AnonymizedFile").default; +const FileModel = require("../src/core/model/files/files.model").default; + +describe("AnonymizedFile custom replacement paths", function () { + let originalFindOne; + let originalFind; + + beforeEach(function () { + originalFindOne = FileModel.findOne; + originalFind = FileModel.find; + }); + + afterEach(function () { + FileModel.findOne = originalFindOne; + FileModel.find = originalFind; + }); + + it("maps a custom replacement back to the original file", async function () { + const original = { + repoId: "repo-1", + path: "src/secret", + name: "index.ts", + sha: "abc", + size: 10, + }; + FileModel.findOne = async () => null; + FileModel.find = () => ({ exec: async () => [original] }); + + const repository = { + repoId: "repo-1", + options: { terms: ["secret=>hidden"] }, + model: { truncatedFolders: [] }, + source: {}, + }; + const file = new AnonymizedFile({ + repository, + anonymizedPath: "src/hidden/index.ts", + }); + + expect(await file.getFileInfo()).to.equal(original); + expect(await file.originalPath()).to.equal("src/secret/index.ts"); + }); +});