fix(git): close diff file-header injection that dropped added lines
ober
b95cf61acc9102d5ece4603617df5451275fc7bd
--- a/gitsafe/git.ss +++ b/gitsafe/git.ss @@ -116,11 +116,13 @@ hunks* (make-diff-hunk current-file 0 new-start '()) new-start)))] - ;; Added line (not +++ header) + ;; Added line. The +++ file header only appears while cur-hunk is + ;; #f (before the first @@), so it is already excluded here; an + ;; added line whose content starts with "++" (git emits "+++ ...") + ;; is content and must be scanned, not dropped. [(and cur-hunk (> (string-length line) 0) - (char=? (string-ref line 0) #\+) - (not (string-prefix? "+++" line))) + (char=? (string-ref line 0) #\+)) (let ([content (substring line 1 (string-length line))] [ln new-line-no]) ;; Append line to current hunk @@ -154,27 +156,94 @@ (def *diff-new-file-re* (re "^\\+\\+\\+ b/(.*)$")) + ;; C-quoted form git emits for non-ASCII / special-character paths: + ;; +++ "b/caf\303\251.txt" + (def *diff-new-file-quoted-re* + (re "^\\+\\+\\+ \"b/(.*)\"$")) + + ;; A --- file-header line immediately precedes its +++ header. Only the real + ;; header forms match (--- a/..., --- /dev/null, --- "..."), so a removed + ;; line inside a hunk is not mistaken for a header. + (def *diff-minus-header-re* + (re "^--- (?:a/|/dev/null|\")")) + + ;; Decode git's C-style quoting (\NNN octal escapes plus backslash escapes) + ;; used inside quoted diff paths. + (def (git-unquote-path s) + (let loop ([cs (string->list s)] [acc '()]) + (cond + [(null? cs) (list->string (reverse acc))] + [(and (char=? (car cs) #\\) (pair? (cdr cs))) + (let ([esc (cadr cs)]) + (if (and (char<=? #\0 esc) (char<=? esc #\7)) + (let octal ([digits (list esc)] [rest (cddr cs)]) + (if (and (< (length digits) 3) + (pair? rest) + (char<=? #\0 (car rest)) + (char<=? (car rest) #\7)) + (octal (append digits (list (car rest))) (cdr rest)) + (loop rest + (cons (integer->char + (string->number (list->string digits) 8)) + acc)))) + (loop (cddr cs) + (cons (case esc + [(#\n) #\newline] + [(#\t) #\tab] + [(#\r) #\return] + [(#\a) #\alarm] + [(#\b) #\backspace] + [(#\f) #\page] + [(#\v) #\vtab] + [(#\\) #\\] + [(#\") #\"] + [else esc]) + acc))))] + [else (loop (cdr cs) (cons (car cs) acc))]))) + + ;; Parse a +++ file-header line. Returns ('file . path) for a real header, + ;; 'deleted for "+++ /dev/null", or #f when LINE is not a +++ header (e.g. an + ;; added line whose content starts with "++"). + (def (parse-plus-header line) + (cond + [(string=? line "+++ /dev/null") 'deleted] + [(re-search *diff-new-file-re* line) + => (lambda (m) (cons 'file (re-match-group m 1)))] + [(re-search *diff-new-file-quoted-re* line) + => (lambda (m) (cons 'file (git-unquote-path (re-match-group m 1))))] + [else #f])) + + ;; A +++ line is a file header only at a genuine file boundary: a --- header + ;; immediately precedes it, and that --- header follows a "diff --git" line. + ;; Content lines always carry a leading +/-/space, so they can never fake a + ;; "diff --git " boundary; this keeps an added line whose content is "++ b/x" + ;; (emitted as "+++ b/x") from being misparsed as a new-file boundary that + ;; would flush the hunk and silently drop every subsequent added line. (def (parse-unified-diff-all diff-text) (let loop ([lines (string-split diff-text #\newline)] [current-file #f] [hunks '()] [cur-hunk #f] - [new-line-no 0]) + [new-line-no 0] + [minus-header? #f] + [after-diff-git? #f]) (if (null? lines) (reverse (flush-hunk hunks cur-hunk)) (let ([line (car lines)] [rest (cdr lines)]) (cond - [(re-search *diff-new-file-re* line) - => - (lambda (m) - (loop rest - (re-match-group m 1) - (flush-hunk hunks cur-hunk) - #f - 0))] - [(string=? line "+++ /dev/null") - (loop rest #f (flush-hunk hunks cur-hunk) #f 0)] + ;; +++ file header — only right after a --- header line. + [(and minus-header? (parse-plus-header line)) + => (lambda (hdr) + (if (eq? hdr 'deleted) + (loop rest #f (flush-hunk hunks cur-hunk) #f 0 #f #f) + (loop rest (cdr hdr) (flush-hunk hunks cur-hunk) #f 0 #f #f)))] + ;; --- file-header line — only right after a diff --git line. + [(and after-diff-git? (re-search *diff-minus-header-re* line)) + (loop rest current-file hunks cur-hunk new-line-no #t #f)] + ;; diff --git marks a file boundary. + [(string-prefix? "diff --git " line) + (loop rest current-file hunks cur-hunk new-line-no #f #t)] [(and current-file (re-search *hunk-header-re* line)) => (lambda (m) @@ -183,11 +252,13 @@ current-file (flush-hunk hunks cur-hunk) (make-diff-hunk current-file 0 new-start '()) - new-start)))] + new-start + #f + #f)))] + ;; Added line (includes "+++ ..." content inside a hunk). [(and cur-hunk (> (string-length line) 0) - (char=? (string-ref line 0) #\+) - (not (string-prefix? "+++" line))) + (char=? (string-ref line 0) #\+)) (let ([content (substring line 1 (string-length line))] [ln new-line-no]) (loop rest @@ -198,17 +269,19 @@ (diff-hunk-old-start cur-hunk) (diff-hunk-new-start cur-hunk) (cons (cons ln content) (diff-hunk-lines cur-hunk))) - (+ new-line-no 1)))] + (+ new-line-no 1) + #f + #f))] [(and cur-hunk (> (string-length line) 0) (char=? (string-ref line 0) #\space)) - (loop rest current-file hunks cur-hunk (+ new-line-no 1))] + (loop rest current-file hunks cur-hunk (+ new-line-no 1) #f #f)] [(and cur-hunk (> (string-length line) 0) (char=? (string-ref line 0) #\-)) - (loop rest current-file hunks cur-hunk new-line-no)] + (loop rest current-file hunks cur-hunk new-line-no #f #f)] [else - (loop rest current-file hunks cur-hunk new-line-no)]))))) + (loop rest current-file hunks cur-hunk new-line-no #f #f)]))))) ;; --- Public API --- --- a/test/test-gitsafe.ss +++ b/test/test-gitsafe.ss @@ -464,8 +464,49 @@ (check-equal? 2 (length hunks)) (check-equal? "a.env" (diff-hunk-file (car hunks))) (check-equal? '(1 . "first") (car (diff-hunk-lines (car hunks)))) - (check-equal? "b.env" (diff-hunk-file (cadr hunks))) - (check-equal? '(3 . "third") (car (diff-hunk-lines (cadr hunks)))))) + (check-equal? "b.env" (diff-hunk-file (cadr hunks))) + (check-equal? '(3 . "third") (car (diff-hunk-lines (cadr hunks)))))) + + (test-case "diff injection: added line '+++ b/...' does not drop later secrets" + ;; An added line whose content is "++ b/decoy.env" is emitted by git as + ;; "+++ b/decoy.env". It must be scanned as content, not misparsed as a + ;; new-file boundary that flushes the hunk and drops following added lines. + (let* ([diff (string-append + "diff --git a/app.env b/app.env\n" + "--- a/app.env\n" + "+++ b/app.env\n" + "@@ -0,0 +1,3 @@\n" + "+++ b/decoy.env\n" + "+aws_key = AKIAIOSFODNN7EXAMPLE\n" + "+padding\n")] + [hunks (parse-unified-diff-all diff)] + [findings (scan-diff-hunks hunks (default-config))] + [aws (filter (lambda (f) (eq? (finding-pattern-id f) 'aws-access-key)) + findings)]) + (check-equal? 1 (length hunks)) + (check-equal? "app.env" (diff-hunk-file (car hunks))) + (check-predicate + (map cdr (diff-hunk-lines (car hunks))) + (lambda (lines) (member "aws_key = AKIAIOSFODNN7EXAMPLE" lines))) + (check-predicate aws pair?))) + + (test-case "diff injection: C-quoted filename is parsed and scanned" + ;; git C-quotes non-ASCII paths: +++ "b/caf\303\251.txt". This must be + ;; recognised as a file header (not dropped) so its hunks are scanned. + (let* ([diff (string-append + "diff --git \"a/caf\\303\\251.txt\" \"b/caf\\303\\251.txt\"\n" + "--- \"a/caf\\303\\251.txt\"\n" + "+++ \"b/caf\\303\\251.txt\"\n" + "@@ -0,0 +1,1 @@\n" + "+key = AKIAIOSFODNN7EXAMPLE\n")] + [hunks (parse-unified-diff-all diff)] + [findings (scan-diff-hunks hunks (default-config))] + [aws (filter (lambda (f) (eq? (finding-pattern-id f) 'aws-access-key)) + findings)]) + (check-equal? 1 (length hunks)) + (check-predicate (diff-hunk-file (car hunks)) + (lambda (f) (and (string? f) (not (string-empty? f))))) + (check-predicate aws pair?))) (test-case "display-findings: SARIF includes rule results and fingerprints" (let* ([c (default-config)]