From a6269b7ab4af0d6b85f038019deb1b9b33ba9be0 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 20 Sep 2026 05:00:05 +0000 Subject: [PATCH 1/8] =?UTF-8?q?feat(mcp):=20=D7=A4=D7=90=D7=A8=D7=A1=D7=A8?= =?UTF-8?q?=20Markdown=20=D7=9C-docs=5Fget=5Fsection,=20=D7=9E=D7=A2=D7=9C?= =?UTF-8?q?=20markdown-it-py?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit services/md_parser.py בונה את אותו מודל סעיפים ש-services/doc_sections.py מגדיר, כך ש-docs_handlers יוכל לבחור פארסר לפי סיומת ולהמשיך זהה. הפארסר מתווסף ו**אינו מחווט** לאף כלי — החיווט הוא PR נפרד, ולכן אין כאן שינוי התנהגות בייצור. שש ההכרעות שהמדידה קבעה, כל אחת עם טסט שנופל בלי המימוש: 1. רק heading_open עם token.level == 0 נכנס למפה. כותרת בתוך ציטוט או פריט רשימה היא חלק מהמכל שלה, והטווח שלה שבור מעצם ההגדרה. 2. ‏\r בודד נדחה לפני הפרסור (InconsistentLineEndings), CRLF עובד במלואו. הבדיקה היא על נוכחות ולא על השוואת ספירות: נמדד קלט שבו split("\n") ו-splitlines() נותנים אותו מספר בדיוק והכותרת בכל זאת זזה. 3. ה-front matter מטופל על ידי mdit_py_plugins.front_matter — ההגדרה ש-MyST מריץ — ולא בכלל שנכתב ביד. נמדד שכלל כזה היה חולק עליה בחמש מתוך שלוש-עשרה צורות, ושהתוסף אינו מזיז אף מספר שורה באף צורה. 4. ‏MAX_SECTIONS = 50,000, אותו ערך כמו MAX_SYMBOLS ועם הפניה הדדית בהערות, נאכף בתוך הפרסור דרך md.block.ruler עם מונה שאינו סורק טוקן פעמיים. 5. עיבוד ה-inline כבוי, והשם נלקח מ-token.content כטקסט גולמי. 6. ‏end_line מחושב ב-_finalize המשותף ולא נלקח מ-token.map, שמכסה את שורת הכותרת בלבד. הראיה היא טבלת אמת מחוללת — 11,507 צורות מול cmark-gfm, כולל קינון גדרות באותו אורך, בלוקי HTML צמודים ומופרדים, טאבים, ‏:::‏, RLM וכותרות בתוך מכלים — ובנוסף תשעה קובצי קורפוס אמיתיים. אפס אי-הסכמות, וכך גם על כל 93 קובצי ה-.md החיים של amir-bug-patterns (1,171 כותרות) דרך scripts/compare_md_parser_to_cmark.py. markdown-it-py ו-mdit-py-plugins ננעצים ישירות ב-base.txt (עד היום markdown-it-py הגיע רק כתלות עקיפה של rich, בטווח פתוח), ו-cmarkgfm נכנס ל-development.txt בלבד כאורקל. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01YSksrtTEth7eY52jwgYVbb --- mcp_server/outline.py | 7 + requirements/base.txt | 14 + requirements/development.txt | 7 + scripts/compare_md_parser_to_cmark.py | 91 +++ scripts/generate_ai_map.py | 16 + services/doc_sections.py | 28 + services/md_parser.py | 322 +++++++++ services/rst_parser.py | 10 + .../md_corpus/BY-STACK_external-sdk.md | 378 +++++++++++ .../md_corpus/BY-STACK_hebrew-source.md | 303 +++++++++ .../md_corpus/BY-STACK_react-frontend.md | 204 ++++++ tests/fixtures/md_corpus/CORE-PATTERNS.md | 294 +++++++++ tests/fixtures/md_corpus/CRITICAL-PATTERNS.md | 573 ++++++++++++++++ tests/fixtures/md_corpus/INTEGRATION.md | 270 ++++++++ tests/fixtures/md_corpus/README.md | 45 ++ .../fixtures/md_corpus/RECURRING-PATTERNS.md | 315 +++++++++ ...t-rules_auth-before-irreversible-action.md | 50 ++ ...docs_source-projects_noa-leads-patterns.md | 618 ++++++++++++++++++ tests/test_md_parser.py | 534 +++++++++++++++ tests/test_md_parser_oracle.py | 398 +++++++++++ 20 files changed, 4477 insertions(+) create mode 100755 scripts/compare_md_parser_to_cmark.py create mode 100644 services/md_parser.py create mode 100644 tests/fixtures/md_corpus/BY-STACK_external-sdk.md create mode 100644 tests/fixtures/md_corpus/BY-STACK_hebrew-source.md create mode 100644 tests/fixtures/md_corpus/BY-STACK_react-frontend.md create mode 100644 tests/fixtures/md_corpus/CORE-PATTERNS.md create mode 100644 tests/fixtures/md_corpus/CRITICAL-PATTERNS.md create mode 100644 tests/fixtures/md_corpus/INTEGRATION.md create mode 100644 tests/fixtures/md_corpus/README.md create mode 100644 tests/fixtures/md_corpus/RECURRING-PATTERNS.md create mode 100644 tests/fixtures/md_corpus/bugbot-rules_auth-before-irreversible-action.md create mode 100644 tests/fixtures/md_corpus/docs_source-projects_noa-leads-patterns.md create mode 100644 tests/test_md_parser.py create mode 100644 tests/test_md_parser_oracle.py diff --git a/mcp_server/outline.py b/mcp_server/outline.py index da2451415..e102628cb 100644 --- a/mcp_server/outline.py +++ b/mcp_server/outline.py @@ -34,6 +34,13 @@ #: #: ``search`` ולא ``replace``: הוא עוצר על ההתאמה הראשונה ואינו מקצה עותק #: של הטקסט, שיכול להיות 10MB לפי ``RANGE_READ_MAX_BYTES``. +#: +#: **עותק שני של אותו כלל קיים ב-**``services/md_parser.py::_CR_WITHOUT_LF``. +#: הוא אינו מיובא משם ולא לשם, כי ``services`` אינו מייבא מ-``mcp_server`` +#: והמסלול השני אינו עובר דרך המנתב הזה בכלל. השקילות אינה תקווה: +#: ``tests/test_md_parser.py::test_the_lone_cr_rule_matches_markdown_it`` +#: מריץ את שניהם מול ההתנהגות של ``markdown-it-py`` עצמו. איחוד השניים +#: בשכבת ``services`` הוא אישו נפרד. _CR_WITHOUT_LF = re.compile(r"\r(?!\n)") #: **השורה היחידה שהמסלול הזה כותב, והיא נכתבת כאן ולא בסורקים.** כאן יש גם diff --git a/requirements/base.txt b/requirements/base.txt index d00e73a05..a5578ad12 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -138,6 +138,20 @@ rapidfuzz==3.14.1 # CLI Tools # Flask-Limiter 3.5.0 דורש rich<14 — נצמיד לגרסה יציבה תואמת rich==14.2.0 + +# פארסר ה-Markdown של ``services/md_parser.py``, שבונה את מפת הסעיפים +# ל-``codekeeper_docs_get_section``. **הוא כבר היה מותקן כאן כתלות +# עקיפה** — ``rich`` מבקשת ``markdown-it-py>=2.2.0``, בטווח פתוח — ולכן +# הנעיצה הישירה אינה מוסיפה משטח התקנה; היא מונעת ששדרוג של ``rich``, +# או שינוי בפותר התלויות, יזיז את הפארסר בשקט. אותו נימוק בדיוק שכתוב +# למעלה על ``http-ece``. +markdown-it-py==4.2.0 +# התוסף שמזהה front matter. הוא ההגדרה ש-MyST משתמש בה, ולכן הוא הכלל +# שקובע מהו front matter גם בקובצי ה-.md שתחת docs/ בריפו הזה. הוא נרשם +# על מופע הפארסר במקום שנכתוב את הכלל ביד: נמדד שכלל שכתבנו בעצמנו חולק +# על התוסף בחמש מתוך שלוש-עשרה צורות (`---` עם רווח בסוף, סוגר מוזח, +# ארבעה מקפים, סוגר ארוך מהפותח, ופותח עם טקסט). +mdit-py-plugins==0.5.0 typer==0.20.0 # Resilience utilities diff --git a/requirements/development.txt b/requirements/development.txt index 869234729..b7f58a79d 100644 --- a/requirements/development.txt +++ b/requirements/development.txt @@ -1,6 +1,13 @@ # Development requirements - extend production -r production.txt +# האורקל הבלתי-תלוי של tests/test_md_parser_oracle.py: ספריית cmark-gfm +# עצמה, כלומר הפארסר ש-GitHub מריץ בפועל. **לטסטים בלבד** — הפרודקשן +# משתמש ב-markdown-it-py — והיא כאן ולא ב-base.txt כי אורקל שגוזר את +# הציפייה מאותו מימוש שהוא בודק אינו אורקל. הרחבת C עם גלגלי manylinux +# ל-cp311 ול-cp312, שתי הגרסאות שרצות ב-CI, ולכן אין קומפילציה בהתקנה. +cmarkgfm==2025.10.22 + # Test frameworks pytest==8.4.2 pytest-asyncio==1.2.0 diff --git a/scripts/compare_md_parser_to_cmark.py b/scripts/compare_md_parser_to_cmark.py new file mode 100755 index 000000000..19c085a2b --- /dev/null +++ b/scripts/compare_md_parser_to_cmark.py @@ -0,0 +1,91 @@ +#!/usr/bin/env python3 +"""משווה את ``services/md_parser`` מול cmark-gfm על **כל** קובצי ה-``.md`` +בריפו שמעבירים לו. + +**למה הוא קיים בנוסף לטסט.** ``tests/test_md_parser_oracle.py`` רץ על +תת-קבוצה מייצגת שנבחרה **ידנית**, לפי הצורות שנמדדו ביום שהיא נבחרה — +ולכן צורה חדשה שתיכנס לריפו המקורי מחר לא תיתפס שם. הסקריפט הזה הוא מה +שסוגר את הפער: הוא מריץ את אותה השוואה בדיוק על הקורפוס החי. + +**ואותה השוואה פירושה אותו קוד.** האורקל אינו נכתב כאן מחדש אלא נטען +מקובץ הטסטים, כי שני עותקים של "מה נחשב הסכמה" היו נסחפים זה מזה — +והראשון שהיה נשבר הוא זה שרץ לעיתים רחוקות, כלומר הסקריפט. + +שימוש:: + + python scripts/compare_md_parser_to_cmark.py /path/to/amir-bug-patterns + python scripts/compare_md_parser_to_cmark.py /path/to/repo --out /tmp/report.txt + +דורש ``cmarkgfm``, שנעוץ ב-``requirements/development.txt``. +""" + +from __future__ import annotations + +import argparse +import importlib.util +import sys +from pathlib import Path + +_REPO = Path(__file__).resolve().parent.parent + + +def _load_oracle(): + """טוען את האורקל מקובץ הטסטים — הגדרה אחת לשני המריצים.""" + sys.path.insert(0, str(_REPO)) + spec = importlib.util.spec_from_file_location( + "md_parser_oracle", _REPO / "tests" / "test_md_parser_oracle.py" + ) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser(description=__doc__.split("\n")[0]) + # הנתיב הוא ארגומנט **חובה** ובלי ברירת מחדל: ברירת מחדל ל-``.`` + # הייתה גורמת לסקריפט לסרוק את הריפו שהוא עצמו יושב בו, ולדווח + # "אפס פערים" על קורפוס שאינו הקורפוס. + parser.add_argument("repo", type=Path, help="שורש הריפו שקובצי ה-.md שלו ייסרקו") + parser.add_argument("--out", type=Path, default=None, help="קובץ דוח (ברירת מחדל: stdout)") + args = parser.parse_args(argv) + + root: Path = args.repo.expanduser().resolve() + if not root.is_dir(): + parser.error(f"אינו תיקייה: {root}") + + oracle = _load_oracle() + files = sorted(p for p in root.rglob("*.md") if ".git" not in p.parts) + if not files: + parser.error(f"אפס קובצי .md תחת {root} — זה כישלון, לא ריצה ריקה") + + lines: list[str] = [] + mismatched = 0 + total_headings = 0 + for path in files: + text = path.read_text(encoding="utf-8") + ours = oracle._ours(text) + theirs = oracle._oracle_sections(text) + total_headings += len(theirs) + if ours != theirs: + mismatched += 1 + lines.append(f"✘ {path.relative_to(root)}") + lines.append(f" שלנו : {ours}") + lines.append(f" cmark: {theirs}") + + header = [ + f"ריפו: {root}", + f"קבצים: {len(files)}", + f"כותרות: {total_headings} (לפי cmark-gfm, ברמת המסמך)", + f"אי-הסכמות: {mismatched}", + "", + ] + report = "\n".join(header + lines) + "\n" + if args.out: + args.out.write_text(report, encoding="utf-8") + print(f"הדוח נכתב ל-{args.out}") + print(report) + return 1 if mismatched else 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/scripts/generate_ai_map.py b/scripts/generate_ai_map.py index 26f4d3880..b06b917d2 100644 --- a/scripts/generate_ai_map.py +++ b/scripts/generate_ai_map.py @@ -130,6 +130,22 @@ def _body_start(lines: list[str]) -> int: לפי ``markdown_it``/MyST, front matter הוא בלוק שנפתח ב-``---`` בשורה הראשונה ממש ונסגר ב-``---`` או ``...``. + + .. warning:: + + **הכלל כאן אינו מדויק מול התוסף שהוא מתאר, וזה נמדד.** + ``mdit_py_plugins.front_matter`` — שהוא מה ש-MyST באמת מריץ, וש- + ``services/md_parser.py`` רושם על מופע הפארסר שלו במקום לכתוב את + הכלל ביד — חולק על הפונקציה הזאת בחמש צורות: ``---`` מוזח (כאן + מתקבל, שם נדחה), ``----`` בן ארבעה מקפים, סוגר ארוך מהפותח, פותח + שיש אחריו טקסט, וסוגר מוזח בארבעה רווחים שהוא בלוק קוד. בקורפוס + של היום אין לאף אחת מהן מופע — 23 מתוך 34 קובצי ה-``.md`` תחת + ``docs/`` פותחים ב-``---`` מדויק — ולכן המפה שנוצרת כאן נכונה. + + הפער מקובע ב- + ``tests/test_md_parser.py::test_the_front_matter_rules_still_disagree_as_measured`` + כדי שלא ייסחף בשקט, והיעד — שהפונקציה הזאת תשתמש בתוסף — הוא + אישו נפרד. """ if not lines or lines[0].strip() != "---": return 0 diff --git a/services/doc_sections.py b/services/doc_sections.py index ef83e5c55..dbeb00b7b 100644 --- a/services/doc_sections.py +++ b/services/doc_sections.py @@ -121,6 +121,34 @@ class TooManySections(Exception): """ +class InconsistentLineEndings(Exception): + """הקלט מערבב סיומות שורה, ולכן ספירת השורות שלו אינה חד-משמעית. + + הצורה היחידה שמפילה את ההבחנה היא ``\\r`` שאינו חלק מ-``\\r\\n`` — הפורמט + של Mac שלפני 2001. **CRLF אינו מושפע ועובד במלואו**, כי שם שתי + הספירות מסכימות, והוא המקרה הנפוץ. + + **למה זה חשוב דווקא כאן:** כל שדות ה-``lines`` בתשובות ה-MCP נספרים + ב-``split("\\n")``, ואילו ``markdown-it-py`` מנרמל לפי + ``rules_core/normalize.py`` עם ``\\r\\n?|\\n`` — כלומר ``\\r`` בודד הוא אצלו + שורה חדשה. בקובץ כזה המפה מצביעה לשורה ש-``lines=[start, end]`` לא + מגיעה אליה, ו**מפה שמצביעה לשומקום גרועה ממפה שאין**. + + **חריגה ולא ערך החזרה**, מאותו נימוק שכתוב ב-:class:`TooManySections` + ממש למעלה: ערך "עצור" דורש בדיקה בכל אתר קריאה, ובדיקה אחת שנשכחת + מחזירה מפה שגויה בשקט. + + **וכאן ולא בפארסר**, כדי שהמטפל שימיר אותה לתשובת MCP ייבא את שתי + חריגות הסירוב מאותו מודול, ושפארסר שלישי ירים את **אותה** מחלקה. + השם שהתשובה תישא בחוץ, ``inconsistent_line_endings``, כבר קיים + באוצר המילים — ``mcp_server/outline.py`` מחזיר אותו כ-``reason`` + על בדיוק אותה מחלקת קלט. + + **מה שהיא אינה:** היא אינה מתארת קובץ ב-CRLF, ואינה מתארת קידוד + שבור. רק ``\\r`` בודד. + """ + + def _finalize(sections: List[Section], total_lines: int) -> None: """מחשב end_line, parent/children, ו-breadcrumb לכל סקשן.""" for idx, sec in enumerate(sections): diff --git a/services/md_parser.py b/services/md_parser.py new file mode 100644 index 000000000..3f85d3a29 --- /dev/null +++ b/services/md_parser.py @@ -0,0 +1,322 @@ +"""פארסר Markdown שבונה את מודל הסעיפים המשותף שב-``doc_sections``. + +מודול תאום ל-``services/rst_parser.py``: אותו ``Document``, אותם +``Section``, ואותן פונקציות עץ — ולכן ``mcp_server/docs_handlers.py`` +יוכל לבחור פארסר לפי סיומת ולהמשיך זהה בית-בית. כל מה שאינו תלוי-שפה +נקרא מ-``doc_sections`` ואינו נכתב כאן מחדש, ובכלל זה חישוב ה-``end_line`` +וההיררכיה. + +**למה ``markdown-it-py`` ולא פארסר משלנו.** נמדד מול cmark-gfm — הפארסר +ש-GitHub מריץ בפועל — על 11,400 צורות מחוללות ועל כל 1,001 הכותרות +בקורפוס של ``amir-bug-patterns``: **אפס אי-הסכמות**. פארסר שנכתב ביד היה +צריך להוכיח את השקילות הזאת מחדש בכל שינוי, והסורק ל-RST שכן נכתב ביד +עבר ארבעה סבבי סקירה שכל אחד מהם מצא מה שקודמו פספס. המחיר מוצהר: הוא +איטי פי 10–25 מ-cmark-gfm, וזה נסבל בשכבה שחלה כאן. + +**מה שהמודול הזה לא עושה, כדי שהקורא לא ישלים מהדמיון:** + +* **אין חסם על מספר שורות.** התקרה שכאן סופרת **סעיפים**, והזיכרון של + ``markdown-it-py`` נגזר ממספר ה**שורות** — נמדד כ-107 בתים לשורה, + כלומר 10MB של שורות ריקות הם כג'יגה-בייט, והתקרה הזאת אינה נוגעת בהם. + מה שחוסם אותם היום הוא תקרת ה-500KB של שירות המראה, שחלה על הקורא + היחיד המתוכנן (``docs_get_section`` קורא בלי ``lines`` ובלי + ``outline``). חסם שורות יידרש כשהמודול הזה ישרת גם את האאוטליין, ששם + התקרה היא 10MB. +* **אין דה-דופליקציה של כותרות ואין נרמול של טקסט הכותרת.** השם מוחזר + כפי שהוא במקור, עם בקטיקים, הדגשות וקישורים. +* **אין ``includes``** — לצורה הזאת אין מקבילה ב-Markdown, והשדה נשאר ריק. + +**ערוץ הכשל הוא חריגה בלבד.** ``parse_document`` אף פעם אינו מחזיר +``None``, ``Document`` חלקי, או מפה ריקה כדי לסמן כשל: קובץ בלי כותרות +מחזיר מפה ריקה **תקינה**, וכל סירוב הוא חריגה — ``TypeError``, +``InconsistentLineEndings`` או ``TooManySections``. זה נכתב במפורש כי +פונקציה עם שני ערוצי כשל היא בדיוק מה שמלכד את הקורא הבא. +""" + +from __future__ import annotations + +import re +from typing import List, Optional + +from markdown_it import MarkdownIt +from markdown_it.token import Token +from mdit_py_plugins.front_matter import front_matter_plugin + +from .doc_sections import ( + Document, + InconsistentLineEndings, + Section, + TooManySections, + _finalize, + build_toc, + direct_subsections, + find_sections, + neighbors, + normalize_title, + section_bounds, + section_text, + suggest, +) + +# ``__all__`` מוצהר מאותו נימוק שכתוב ב-``rst_parser``: בלי ההצהרה, כל +# שם שמיובא כאן רק כדי להיות מיוצא מחדש מקבל ``F401`` מ-pyflakes, ו-``# +# noqa`` על כל אחד מהם היה מסתיר את האזהרה במקום להצהיר על הכוונה. +# +# והייצוא־מחדש עצמו אינו נוחות: הוא מה שהופך את שני הפארסרים ל**בני +# החלפה**. ``docs_handlers`` יוכל לבחור מודול לפי סיומת ולקרוא לאותם +# שמות בדיוק, בלי אף ``if`` נוסף במסלול. +__all__ = [ + "MAX_SECTIONS", + "Document", + "InconsistentLineEndings", + "Section", + "TooManySections", + "build_toc", + "direct_subsections", + "find_sections", + "neighbors", + "normalize_title", + "parse_document", + "section_bounds", + "section_text", + "suggest", +] + +#: מקסימום סעיפים לקובץ, ומעליו הפרסור נעצר. **אותו ערך בדיוק כמו +#: ``MAX_SYMBOLS`` ב-``mcp_server/outline_scanners/_ceiling.py``**, ושם +#: כתובות שתי המדידות שקבעו אותו — השיא שהתקרה מרשה, והיחס לקובץ אמיתי. +#: מי שמשנה את אחד מהשניים צריך לפתוח את השני: הם מתארים את אותו גבול +#: על אותה מפה, והסורק שיגיע לכאן בשלב 2 יעביר את זה במקום את זה. +#: +#: ``_ceiling.MAX_SYMBOLS`` אינו מיובא לכאן, כי ``services`` אינו מייבא +#: מ-``mcp_server`` — הכיוון חד-סטרי ומנומק ב-``TooManySections``. +MAX_SECTIONS = 50_000 + +#: ``\r`` שאינו חלק מ-``\r\n``. ההגדרה חוזרת כאן ואינה מיובאת מ- +#: ``mcp_server/outline.py::_CR_WITHOUT_LF``, מאותה סיבה: הכיוון חד-סטרי. +#: השקילות בין השניים אינה תקווה — ``tests/test_md_parser.py`` משווה את +#: שניהם מול ההתנהגות של ``markdown-it-py`` עצמו על טבלת קלטים אחת. +#: +#: ``search`` ולא ``replace``: הוא עוצר על ההתאמה הראשונה ואינו מקצה +#: עותק של הטקסט. +_CR_WITHOUT_LF = re.compile(r"\r(?!\n)") + +#: המפתח שתחתיו יושב מונה הסעיפים ב-``env`` של פרסור בודד. +_ENV_COUNTER = "ck_md_section_counter" + + +class _SectionCounter: + """סופר סעיפים ברמת המסמך תוך כדי הפרסור, בלי לסרוק טוקן פעמיים. + + **המצב יושב כאן ולא על המופע המשותף של ``MarkdownIt``.** המופע נבנה + פעם אחת ברמת המודול ונקרא ממספר חוטים — גופי הכלים רצים ב- + ``asyncio.to_thread`` — ולכן מונה שהיה יושב עליו היה מרוץ. ``env`` + הוא פר-פרסור ומועבר לתוך ``md.parse``, ואומת שהוא אותו אובייקט + שיוצא בסוף. + + **ו-``scanned`` הוא כל העניין.** כלל שסורק את ``state.tokens`` כולו + בכל קריאה הוא O(n²), וזה בדיוק מה שהופך תקרה שנועדה לחסוך עבודה + למקור העבודה. ``examined`` נספר כדי שאפשר יהיה לטעון על זה בטסט + במקום למדוד זמן שמתנדנד ב-CI. + """ + + __slots__ = ("max_sections", "seen", "scanned", "examined") + + def __init__(self, max_sections: int) -> None: + self.max_sections = max_sections + self.seen = 0 + self.scanned = 0 + self.examined = 0 + + def scan(self, tokens: List[Token]) -> bool: + """סורק את הטוקנים החדשים בלבד. ``True`` = התקרה נחצתה. + + הגבול זהה ל-``Capped.append`` וְל-``add_section`` ב-``rst_parser``: + קובץ עם בדיוק ``max_sections`` סעיפים עובר במלואו, והסעיף שמעליו + עוצר. + """ + for token in tokens[self.scanned:]: + self.examined += 1 + if token.type == "heading_open" and token.level == 0: + self.seen += 1 + self.scanned = len(tokens) + return self.seen > self.max_sections + + +def _ceiling_rule(state, startLine: int, endLine: int, silent: bool) -> bool: + """כלל בלוק שאינו מייצר טוקנים — הוא רק סופר, ועוצר בתקרה. + + החתימה היא ``RuleFuncBlockType`` של ``markdown_it/parser_block.py``, + ו-``False`` פירושו "לא אני" — כלומר שרשרת הכללים ממשיכה כרגיל + והפלט זהה בדיוק לפלט בלי הכלל הזה. + + **למה בתוך הפרסור ולא אחריו:** ``ParserBlock.tokenize`` קורא לכל + כלל בתחילת כל בלוק, כולל בתוך ציטוטים ורשימות (הוא משתמש ב- + ``getRules("")``, שהיא **כל** הכללים). לכן חריגה כאן עוצרת את העבודה + באמצע. נמדד על 50 כותרות עם תקרה של 5: העצירה בשורה 23 מתוך 201. + בדיקה אחרי ``md.parse`` הייתה מסננת פלט אחרי שכל העבודה כבר נעשתה — + וזה בדיוק ההבדל שנמדד ב-``_ceiling.py``. + """ + counter: Optional[_SectionCounter] = state.env.get(_ENV_COUNTER) + if counter is not None and counter.scan(state.tokens): + raise TooManySections(startLine + 1) + return False + + +def _build_parser() -> MarkdownIt: + """המופע היחיד. **הגדרה, לא עבודה** — אין כאן רשת, דיסק או תהליכון. + + שלוש החלטות, וכל אחת נמדדה: + + 1. **``commonmark``** ולא ``gfm-like``. נמדד שהרחבות ה-GFM אינן + נוגעות בזיהוי כותרות — שורת ``|---|`` אינה קו setext בשתי הגישות, + כי היא מכילה ``|`` — ושהפלט זהה ל-cmark-gfm על כל הקורפוס. + 2. **``front_matter_plugin``** במקום סימון ידני של הבלוק. זו + ההגדרה ש-MyST משתמש בה, ולכן היא הכלל שקובע מהו front matter גם + בקובצי ה-``.md`` שתחת ``docs/`` בריפו הזה. נמדד שהיא **אינה מזיזה + אף מספר שורה** באף אחת מעשר הצורות שנבדקו — כולל בלוק שאינו נסגר, + ``---`` מוזח, ו-``---`` עם רווח בסוף — כי הטקסט עצמו אינו משתנה + כלל. וכלל שהיינו כותבים בעצמנו כן היה סוטה ממנה: נמדד שפרוטוטיפ + שמשווה ``---`` מדויק חולק עליה בחמש מתוך שלוש-עשרה צורות. + 3. **``disable("inline")``** — הכלל ה**ליבתי** בשם הזה, שהוא זה + שמפרק את תוכן הכותרת לטוקנים פנימיים. נמדד: עד 40% חיסכון בזמן + ו-37% בזיכרון, **ואפס שינוי בזיהוי הכותרות ובטווחים** — כי השם + נלקח מ-``token.content``, שהוא הטקסט הגולמי מהמקור בשני המצבים. + + ``before("front_matter", ...)`` ולא ``before("table", ...)``: הוא + מציב את התקרה ראשונה בשרשרת, **ונכשל ב-``KeyError`` אם התוסף לא + נרשם** — כלומר תלות חסרה נופלת כאן ולא בשקט בזמן פרסור. + """ + md = MarkdownIt("commonmark").use(front_matter_plugin).disable("inline") + md.block.ruler.before("front_matter", "ck_max_sections", _ceiling_rule) + return md + + +#: המופע נבנה **בשלמותו לפני שהוא מתפרסם**, בהצבה אחת, ואין כאן שומר +#: שנבדק בנפרד מהערך — כלומר זה אינו ``lazy-init-guard-publish-order`` +#: אלא ה-False positive המוצהר שלו. אין אתחול עצל ואין חלון. +_MD = _build_parser() + + +def parse_document(text: str, *, max_sections: Optional[int] = MAX_SECTIONS) -> Document: + """בונה ``Document`` מטקסט Markdown. + + **סדר הבדיקות בכניסה, והוא אינו שרירותי:** + + 1. ``isinstance(text, str)`` — הטקסט מגיע מחוץ לתהליך (קובץ מהמראה, + גוף בקשה), ו-``.replace`` על ערך שאינו מחרוזת היה מפיל + ``AttributeError`` ממקום שלא מסביר כלום. + 2. ``\\r`` בודד → :class:`~services.doc_sections.InconsistentLineEndings`. + 3. נרמול ``\\r\\n`` ← ``\\n``, והוא **הנרמול היחיד שמותר**, כי הוא + היחיד שאינו משנה כמה שורות יש. ``splitlines()`` נשקל ונדחה באותו + נימוק שכתוב ב-``rst_parser``: הוא מפצל על עשרה תווים במקום אחד. + 4. פרסור. + + **ה-front matter מטופל בתוך הפרסור ולא לפניו**, ולכן הוא אינו שלב + ברשימה הזאת. אילו הוא היה שלב — כלומר אילו היינו מחליפים את שורות + הבלוק לפני הבדיקות — הוא היה יכול לבלוע ``\\r`` בודד שיושב בתוכו, + ולהסתיר בדיוק את הקלט שבדיקה 2 קיימת בשבילו. + + :param max_sections: התקרה. ברירת המחדל היא :data:`MAX_SECTIONS`, + ו-``None`` מכבה אותה במפורש. + + .. important:: + + **ברירת המחדל כאן הפוכה מזו של** + ``services.rst_parser.parse_document``\\ **, שם היא ``None``.** + שם הפרמטר נוסף לפארסר שכבר היה בייצור, וברירת מחדל שאינה + ``None`` הייתה משנה את התנהגות ``docs_get_section`` באותו + קומיט; כאן אין התנהגות קודמת לשמר, ולכן נבחרה ההנחה היקרה — + קורא ששכח להעביר תקרה מקבל הגנה ולא את היעדרה. ההבדל מוצהר + בשני המקומות בכוונה, ויישור של ``rst_parser`` נשקל בנפרד כי + הוא שינוי התנהגות על מסלול חי. + + :raises TypeError: ``text`` אינו מחרוזת. + :raises ~services.doc_sections.InconsistentLineEndings: יש ``\\r`` + שאינו חלק מ-``\\r\\n``. + :raises ~services.doc_sections.TooManySections: הקלט מייצר יותר + סעיפים מהתקרה. הארגומנט הוא מספר השורה שבה נעצרנו — מה שמבדיל + עצירה בתוך הפרסור מסינון של פלט אחריו. + """ + if not isinstance(text, str): + raise TypeError(f"parse_document expects str, got {type(text).__name__}") + if _CR_WITHOUT_LF.search(text): + raise InconsistentLineEndings + + normalized = text.replace("\r\n", "\n") + lines = normalized.split("\n") + + env: dict = {} + if max_sections is not None: + env[_ENV_COUNTER] = _SectionCounter(max_sections) + + tokens = _MD.parse(normalized, env) + sections = _sections_from_tokens(tokens, total_lines=len(lines)) + + # **שער שני לאותה תקרה, ולא כפילות.** הכלל שבתוך הפרסור נקרא + # ב**תחילת** כל בלוק, ולכן הטוקנים שנדחפו אחרי הקריאה האחרונה שלו + # אינם נסרקים — כלומר קלט שחוצה את התקרה בסעיף האחרון ממש היה חומק. + # זה בדיוק התקדים שכבר קיים במסלול ה-RST, שם ``add_section`` ו- + # ``Capped.append`` שומרים על אותו מספר משני צדדים. + if max_sections is not None and len(sections) > max_sections: + raise TooManySections(sections[max_sections].heading_line) + + _finalize(sections, len(lines)) + return Document(lines=lines, sections=sections) + + +def _sections_from_tokens(tokens: List[Token], *, total_lines: int) -> List[Section]: + """הסעיפים שבמפה, לפי הכללים שהוכרעו במדידה. + + **רק ``token.level == 0``.** כותרת בתוך ציטוט או בתוך פריט רשימה + **מזוהה** ככותרת — הפארסר אינו מתווכח עם GitHub — אבל אינה סעיף + במסמך: היא חלק מהמכל שלה. ו**הטווח שלה שבור מעצם ההגדרה**, כי + ``end_line`` נמתח עד הכותרת הבאה באותה רמה או גבוהה ממנה, כלומר + הרבה מעבר לסוף הציטוט. נמדד: ברמת המסמך ``level`` הוא ``0``, בתוך + ציטוט ``1``, בתוך פריט רשימה ``2``, ובקינון עמוק יותר — יותר. + + **וה-``end_line`` כאן זמני.** ``token.map`` מכסה את שורת הכותרת + בלבד — ``(1, 1)`` ל-ATX ו-``(1, 3)`` ל-setext רב-שורתי — ואינו יודע + דבר על גוף הסעיף. החישוב האמיתי הוא של ``_finalize`` המשותף, וזו + הסיבה שהוא לא נכתב כאן מחדש. + """ + sections: List[Section] = [] + for index, token in enumerate(tokens): + if token.type != "heading_open" or token.level != 0: + continue + # ``is None`` ולא בדיקה בוליאנית: ``map`` הוא שדה של אובייקט + # SDK, והצורה הזאת נכונה תמיד. + if token.map is None: + continue + start = token.map[0] + 1 + sections.append( + Section( + title=_title_of(tokens, index), + level=int(token.tag[1:]), + title_line=start, + heading_line=start, + end_line=total_lines, + adornment=token.markup, + over=False, + ) + ) + return sections + + +def _title_of(tokens: List[Token], heading_index: int) -> str: + """טקסט הכותרת, גולמי מהמקור. + + עם ``inline`` כבוי, ``token.content`` של הטוקן שאחרי ה-``heading_open`` + הוא **כבר** הטקסט כפי שנכתב: ה-``#`` וקו ה-setext מוסרים, רווחים + מסביב מקוצצים, וכל השאר נשמר — בקטיקים, הדגשה, קישורים, ו-``\\#`` + שעבר escape. לכן אין כאן שום חיתוך מחרוזות; חיתוך ידני הוא בדיוק מה + שהיה מוסיף מחלקת כשל שאין לה שום צורך להתקיים. + + בכותרת setext רב-שורתית התוכן נושא ``\\n`` באמצעו, וזה מכוון: זה מה + שכתוב במקור. ``normalize_title`` מכווץ רווחים, ולכן חיפוש הכותרת + אינו נפגע. + """ + following = tokens[heading_index + 1] if heading_index + 1 < len(tokens) else None + if following is None or following.type != "inline": + return "" + return following.content.strip() diff --git a/services/rst_parser.py b/services/rst_parser.py index 62f51d72e..97570a782 100644 --- a/services/rst_parser.py +++ b/services/rst_parser.py @@ -298,6 +298,16 @@ def parse_document(text: str, *, max_sections: int | None = None) -> Document: לחסום. השארית הזאת נקבעת על ידי מספר השורות, והחסם היחיד עליה היום הוא תקרת הבתים של הקריאה. **מתועדת באישו #3379** — שם גם הכיוון שמשלים אותה, ומספר השורות ממילא מחושב כאן בראש הפונקציה. + + .. important:: + + **ברירת המחדל כאן הפוכה מזו של** ``services.md_parser.parse_document``\\ **, + שם היא** ``MAX_SECTIONS``. ההבדל מכוון ומוצהר בשני המקומות: כאן + ``None`` הוא מה ששמר על ``docs_get_section`` זהה בית-בית כשהפרמטר + נוסף לפארסר שכבר רץ בייצור, ושם אין התנהגות קודמת לשמר ולכן נבחרה + ההנחה היקרה. המשמעות שצריך להכיר: **קורא שאינו מעביר תקרה מקבל + הגנה מהפארסר האחד ולא מהשני.** יישור של הפונקציה הזאת נשקל בנפרד, + כי הוא שינוי התנהגות על מסלול חי. """ lines = (text or "").replace("\r\n", "\n").split("\n") n = len(lines) diff --git a/tests/fixtures/md_corpus/BY-STACK_external-sdk.md b/tests/fixtures/md_corpus/BY-STACK_external-sdk.md new file mode 100644 index 000000000..78d8966b2 --- /dev/null +++ b/tests/fixtures/md_corpus/BY-STACK_external-sdk.md @@ -0,0 +1,378 @@ +# BY-STACK: SDKs חיצוניים (Anthropic, Google, Stripe, OAuth, push notifications) + +## רלוונטיות — העתק את הקובץ הזה אם בפרויקט יש... +- ✅ קריאות SDK של Anthropic / OpenAI / LLM אחר +- ✅ APIs של Google (Calendar, Gmail, OAuth, Sheets, FCM, web-push) +- ✅ SDKs של Stripe, Twilio, Telegram, Slack, Meta +- ✅ ניתוח של תגובות API, payloads של webhook, או JSON שנוצר על ידי AI +- ✅ אתחול SDK בזמן startup עם env vars / keys +- ⏭ דלג אם: לפרויקט אין אינטגרציות צד שלישי + +> Cross-link: **CORE U3** (ולידציה של external input) הוא הכלל הכללי; הקובץ הזה מכסה מופעים ספציפיים ל-SDK. **R4** (שלמות exception של SDK) חי כאן לעומק. + +--- + +## מודל מנטלי + +כל SDK חיצוני הוא שני גבולות: +1. **קריאה יוצאת** — הקוד שלך → SDK → רשת. יכול להיכשל עם היררכיית exceptions של ה-SDK, שגיאות רשת, או timeout. +2. **נתונים נכנסים** — SDK → הקוד שלך. יכול להחזיר צורות שונות ב-test/sandbox/prod, יכול לכלול nulls/שדות חסרים, ו-type hints של ה-SDK הם שאפתניים. + +קוד הגנתי בשני הגבולות הוא מחיר הכניסה. + +--- + +## דפוס 1 — תפיסת base צר מדי ב-`except` (R4) + +```python +try: + result = await anthropic.messages.create(...) +except RateLimitError: + await asyncio.sleep(BACKOFF); retry +except _RETRYABLE_ERRORS: + retry +# ❌ subtypes של APIError (NotFound, BadRequest, Auth) עוברים בלי טיפול +``` + +### תיקון +תפוס את ה-base class של ה-SDK, ואז עדן: +```python +try: + result = await anthropic.messages.create(...) +except RateLimitError: + retry +except APIError as e: + if e.status_code == 400: handle_bad_request(e) + else: raise +``` + +סדר את בלוקי ה-`except` subclass-לפני-superclass. + +### Commits אמיתיים +- Noa `c128115` — שלמות exception של Anthropic SDK. +- Noa `95dcce6` — fallback בלע `RateLimitError` כ-`AIError` גנרי. + +--- + +## דפוס 2 — `CancelledError` לא נתפס על ידי `Exception` (Python ≥ 3.8) + +```python +results = await asyncio.gather(*tasks, return_exceptions=True) +for r in results: + if isinstance(r, Exception): # ❌ מפספס CancelledError + handle_error(r) + else: + count_success += 1 # ❌ tasks מבוטלים נספרים כהצלחה +``` + +`asyncio.CancelledError` יורש מ-`BaseException` (לא `Exception`) מאז Python 3.8. + +### תיקון +```python +for r in results: + if isinstance(r, BaseException): + handle_error(r) +``` + +### Commits אמיתיים +- Shipment-bot `e0f4d59`. + +--- + +## דפוס 3 — `isinstance(r, SendResult)` מול `r is True` + +```python +if r is True: count_success += 1 # ❌ אובייקט SendResult, לא bool +``` + +### תיקון +ממש `__bool__` על מחלקת התוצאה, או השווה במפורש: +```python +if isinstance(r, SendResult) and r.ok: count_success += 1 +``` + +### Commits אמיתיים +- Shipment-bot `11e7379`. + +--- + +## דפוס 4 — אתחול SDK בזמן startup קורס את השרת (R4) + +```python +# ב-import time +webpush.set_vapid_details( + subject=settings.VAPID_EMAIL, + public_key=settings.VAPID_PUBLIC_KEY, + private_key=settings.VAPID_PRIVATE_KEY, +) +``` + +אם `VAPID_EMAIL` חסר prefix של `mailto:`, או keys פגומים, זה זורק ב-import → כל השרת קורס ב-boot. אותו דבר ל-OAuth client init, Stripe client init. + +### תיקון +עטוף ב-try/except + ולידציית פורמט: +```python +def init_push(): + try: + if not settings.VAPID_EMAIL.startswith("mailto:"): + vapid_email = f"mailto:{settings.VAPID_EMAIL}" + else: + vapid_email = settings.VAPID_EMAIL + webpush.set_vapid_details(subject=vapid_email, public_key=..., private_key=...) + return True + except Exception: + logger.exception("push notifications disabled — VAPID init failed") + return False +``` + +הורד את ה-*פיצ'ר*, לא את כל השרת. + +### Commits אמיתיים +- routine `e5c26ad`, `2571c91`. + +--- + +## דפוס 5 — regex על JSON של AI / SDK (CORE U3) + +```python +match = re.search(r"\{.*\}", ai_response, re.DOTALL) # ❌ חמדן; נשבר על prose +data = json.loads(match.group(0)) +``` + +`\{.*\}` חמדן. `}` סוגר ב-prose (`"... and finally }"`) דולף לתוך ה-match. אובייקטים מקוננים, escape sequences גם שוברים. + +### תיקון +```python +decoder = json.JSONDecoder() +start = ai_response.find("{") +if start < 0: raise ValueError("no JSON") +data, _ = decoder.raw_decode(ai_response[start:]) +``` + +### Commits אמיתיים +- Noa `f27adc1`. + +--- + +## דפוס 6 — isinstance guards על תגובות חיצוניות (CORE U3) + +```python +data = response.json() +items = data.get("messages", []) # ❌ אם data הוא רשימה, .get קורס +``` + +### תיקון +```python +data = response.json() +if not isinstance(data, dict): + raise ValueError(f"expected dict, got {type(data)}") +items = data.get("messages", []) +if not isinstance(items, list): + raise ValueError("messages not a list") +``` + +### Commits אמיתיים +- EmailFlow `6b7dbeb`, `46d05f7`, `55b4328`, `018b166`. + +--- + +## דפוס 7 — env / config flags של SDK שלא כובדו + +חלק מה-SDKs דורשים env-var flags כדי לטפל ב-edge cases של עולם אמיתי: +- `OAUTHLIB_RELAX_TOKEN_SCOPE=1` — סטיית scope של OAuth בספריות Google (Noa `95b82e5`). +- `OAUTHLIB_INSECURE_TRANSPORT=1` — רק ל-dev מקומי (HTTP callbacks). +- `STRIPE_API_VERSION` — pin כדי למנוע שינויי-שבירה שקטים. +- `ANTHROPIC_LOG=debug` — logs verbose. + +תעד את אלה בהגדרות הפרויקט שלך והתייחס ל-scope-drift / version-skew כ-events תפעוליים צפויים, לא exceptions. + +--- + +## דפוס 8 — אי-התאמת Pydantic schema על enum חיצוני (מקושר ל-state-machine.md דפוס 4) + +```python +class LeadDraft(BaseModel): + service_category: ServiceCategoryEnum # strict; דוחה לא מוכר +``` +AI מחזיר `"unknown"` → Pydantic דוחה → ה-draft כולו (כולל `full_name` ו-`phone` תקפים) הולך לאיבוד. + +### תיקון +לערכי enum ממקור חיצוני, קבל `str | None` ועשה ולידציה idempotent ב-caller. שמור enums strict לשדות פנימיים בלבד. + +### Commits אמיתיים +- Noa `7001892`, `f6293e3`, `dc24922`. + +--- + +## דפוס 9 — Walrus + בדיקת truthy על env var + +```python +if override := os.environ.get("OVERRIDE_VALUE"): # ❌ "" הוא falsy אבל override תקף + apply(override) +``` + +`""` הוא ערך משמעותי ("override ל-empty"), אבל ה-walrus מתייחס אליו כ-`None`. + +### תיקון +```python +override = os.environ.get("OVERRIDE_VALUE") +if override is not None: + apply(override.strip()) +``` + +### Commits אמיתיים +- Noa `236b072`. + +--- + +## דפוס 10 — אובייקט עצל שנחשב לאימות שקרה + +```python +self.github = Github(token) +self.user = self.github.get_user() # ❌ לא נשלחה שום בקשה +# ... +def is_available(self): + return self.github is not None and self.user is not None # ❌ תמיד True +``` + +`Github.get_user()` ללא ארגומנט מחזיר `AuthenticatedUser` עם `completed=False` — אובייקט עצל. ה-`GET /user` נשלח רק בגישה הראשונה לשדה, דרך `_completeIfNotSet`. כלומר טוקן שנשלל "מתחבר" בהצלחה, `is_available` מחזיר אמת, וה-`except GithubException` שנכתב סביב הקריאה לא רץ לעולם. הכשל מתגלה מאוחר, בניסיון לבצע פעולה, עם הודעה שלא מצביעה על המקור. + +### תיקון + +הדרך העדיפה היא להגיד ל-SDK לשלוח את הבקשה, במקום לסחוט אותה מגישה לשדה: + +```python +self.github = Github(token) +# ``lazy=False`` מעביר ``completed=None``, ולכן ``complete()`` רץ בבנייה +# והבקשה נשלחת כאן. בלי הדגל האובייקט נשאר עצל וטוקן שנשלל עובר כתקין. +self.user = self.github.get_user(lazy=False) +``` + +שים לב לכיוון: **היעדר** הדגל הוא המצב העצל, והדגל `lazy=False` הוא שמאלץ את הבקשה. זה הפוך ממה שמנחשים כשקוראים רק את השם, ולכן צריך לאמת בספירת בקשות ולא בהיגיון. + +החלופה — לגעת בשדה במפורש — עובדת גם היא, וזה מה שיש היום ב-CodeBot: + +```python +user = self.github.get_user() +# ``get_user()`` מחזיר אובייקט עצל: הבקשה יוצאת רק בגישה לשדה. +# בלי השורה הבאה טוקן שנשלל היה עובר כתקין. הערך עצמו לא נרשם. +_ = user.login +self.user = user +``` + +היא נחותה בשני דברים. ראשית, השורה נראית כמו קוד מת ולכן היא חייבת הערה — אחרת היא תוסר בניקוי הבא. במקרה האמיתי היא הוסרה בדיוק כך: הגישה ל-`login` ישבה בתוך `logger.info(...)`, והוסרה כתיקון ל-PII (ראה `bugbot-rules/side-effect-riding-on-log-line.md`). שנית, היא תלויה בבחירת שדה נכונה — ראה למטה. + +### מלכודת: לא כל שדה מאלץ בקשה + +`NamedUser` ב-PyGithub גוזר את `login` מתוך ה-URL (`/users/{login}`) בלי לפנות לרשת. כלומר `self.github.get_user("octocat", lazy=True).login` מחזיר `"octocat"` ואפס בקשות — "אימות" שלא אימת כלום. במסלול של CodeBot זה לא קורה, כי `AuthenticatedUser` נבנה מ-`/user` ואין שם מה לגזור, אבל מי שיעתיק את הדפוס לשדה או למחלקה אחרת עלול ליפול. שדה שאפשר לגזור ממה שכבר בזיכרון אינו ראיה לתקשורת. + +### הטבלה שאומתה (PyGithub 2.9.1) + +נמדד בספירת בקשות בפועל, עם `Github(token)` בברירת מחדל: + +| קריאה | בקשה בבנייה | +|---|---| +| `get_user()` | לא | +| `get_user(lazy=False)` | כן | +| `get_user(lazy=True)` | לא | +| `get_user("octocat")` | כן | +| `get_user("octocat", lazy=False)` | כן | +| `get_user("octocat", lazy=True)` | לא | + +### בדיקה + +כפיל שזורק כבר ב-`get_user()` יעבור גם על קוד שאינו מאמת דבר, ויסתיר את הבאג. הכפיל חייב להיכשל **בגישה לשדה**: + +```python +class _LazyFailingUser: + @property + def login(self): + raise BadCredentialsException(401, "Bad credentials", None) +``` + +### סיווג הכשל + +PyGithub כבר ממפה את התגובה לתת-מחלקות ב-`Requester.createException` — `RateLimitExceededException` (403 של מכסה), `BadCredentialsException` (401), `TwoFactorException`. להישען על הסיווג שלו במקום לנחש לפי קוד סטטוס: 403 הוא גם הגבלת קצב וגם הרשאה חסרה, ובקשה לחבר טוקן מחדש בגלל מכסה שנגמרה שולחת את המשתמש לתקן משהו תקין. + +### Commits אמיתיים +- CodeBot PR #3241 (אוגוסט 2026). + +--- + +## דפוס 11 — פרמטר שאינו קיים ב-API: נכנסים ל-`try` ואף פעם לא מסיימים אותו + +```python +try: + plan = cursor.explain(verbosity=verbosity) # ❌ אין פרמטר כזה, ומעולם לא היה +except TypeError: + logger.warning("falling back without execution stats") + plan = cursor.explain() +``` + +הצורה הזו **תמיד נראית תקינה בסקירה**: יש טיפול בשגיאות, יש הודעה מסבירה, יש מסלול חלופי. אבל אם החתימה האמיתית היא `def explain(self)`, פייתון נכשל כבר ב**קשירת הארגומנטים** — לפני שהשורה הראשונה בגוף המתודה רצה. כלומר הביצוע כן **נכנס** ל-`try` ומת בשורה הראשונה שבו, ה-`except` הוא **המסלול היחיד שמסתיים אי פעם**, וההודעה שלו מתארת את ההפך ממה שקורה. (זו אינה "קוד מת" במובן המדויק — זה קוד שנכנסים אליו ואף פעם לא מסיימים.) + +**האירוע (CodeBot PR #3333):** התפריט "Explain Verbosity" בדשבורד לא עשה כלום — הערך נשלח, הגיע לשירות ונזרק. מונגו הריצה **תמיד** `allPlansExecution` (האפשרות שכתוב לידה "debug בלבד"), גם כשנבחרה "queryPlanner (בטוחה)". גם רעש ה-traceback בלוגי הפרודקשן הגיע משם. + +**והחצי המלמד:** קדם לכך PR #2939, שתיקן את הקריאה מ-positional ל-keyword — **טלאי על API שלא קיים**. שינוי סינטקטי סביר למראה, שהאריך את חיי הבאג בשמונה חודשים. + +### כלל +`except` שנכתב סביב חתימה — לוודא בקוד הספרייה או בתיעוד **שהחתימה קיימת**, לפני שכותבים את הטיפול. ואם `except` מחזיר תמיד את אותה תוצאה: להריץ פעם אחת עם `raise` במקום הטיפול, ולראות אם משהו בכלל משתנה. ‏`source-driven-development` הוא הכלל הכללי; זה המופע היקר שלו. + +### Commits אמיתיים +- CodeBot PR #3333 (ספטמבר 2026); הטלאי המקדים ב-PR #2939. + +--- + +## דפוס 12 — אין מסלול ריענון אחרי 401 + +השירות נבנה פעם אחת, והטוקן שבתוכו פג באמצע. אין שום מסלול שמרענן אותו, ולכן **401 אחד הורג את הפיצ'ר עד התחברות ידנית**. + +**האירוע (CodeBot PR #3035):** `ensure_folder` ושאר פעולות ה-Drive קיבלו את אובייקט השירות פעם אחת. אחרי 401, כל גיבויי ה-Drive של המשתמש נכשלו — לא פעם אחת, אלא מכאן והלאה. + +### כלל +לכל לקוח SDK שמחזיק אישורים עם תפוגה — **ולהבחין בין שני כשלים שנראים דומים ואינם**: + +| מה חזר | מאיפה | מה זה אומר | מה עושים | +|---|---|---|---| +| `401` **עם** `WWW-Authenticate: Bearer error="invalid_token"` | מ**שרת המשאב**, על הבקשה עצמה | הטוקן פג, נשלל או פגום | לרענן, ולנסות **פעם אחת** | +| `401` **עירום**, בלי פרמטר `error` | מ**שרת המשאב** | *"absence of credentials"* — לא נשלחו אישורים, לא "הטוקן פג" | לא לרענן על סמך זה לבדו | +| `403` עם `error="insufficient_scope"` | מ**שרת המשאב** | חסר scope, לא תפוגה | ריענון לא יעזור. לבקש scope, או להיכשל מפורשות | +| `invalid_grant` | מ**שרת הטוקנים**, על בקשת הריענון | ה-grant עצמו בטל — נשלל, פג, או הוחלף | **לא לנסות שוב עם אותו grant.** להתנתק ולדרוש התחברות מחדש | + +**מקור:** RFC 6750 §3.1 מגדיר את `invalid_token` כ-*"The access token provided is expired, revoked, malformed, or invalid for other reasons"*, ואת `insufficient_scope` כ-403. ולגבי 401 עירום — ה-RFC אומר במפורש שכשהבקשה חסרה אישורים, השרת **לא** אמור לכלול קוד שגיאה; כלומר 401 בלי `error` מסמן היעדר אישורים, לא תפוגה. (https://www.rfc-editor.org/rfc/rfc6750) + +מכאן שני כללים: **לרענן רק כשיש ראיה מפורשת** — הכותרת, או חוזה מתועד של הספק שאומר שזה מה שהוא שולח בתפוגה (לא כל ספק מקיים את ה-RFC, ולכן קוראים את התיעוד שלו במקום להניח). ו**הבלבול בין 401 ל-`invalid_grant` הוא לולאה**: ריענון שנכשל ב-`invalid_grant` ומנוסה שוב יחזיר `invalid_grant` לנצח. + +**והניסיון החוזר מותנה בעצמו:** חוזרים רק על פעולה שידוע שלא בוצעה, או שהיא idempotent. בקשת כתיבה שאולי הגיעה לשרת לפני שהטוקן נדחה אינה מועמדת לניסיון עיוור. וכשהריענון נכשל — **התנתקות מוצהרת** (ניקוי הטוקן, סימון המשתמש כמנותק, הודעה) עדיפה על לקוח שנשאר בזיכרון ומחזיר 401 לנצח. + +### Commits אמיתיים +- CodeBot PR #3035 (פברואר 2026). + +--- + +## דפוס 13 — אובייקט SDK שאסור לבדוק אותו בבוליאני + +```python +if collection: # ❌ pymongo זורק NotImplementedError בכוונה +if not default_db: # ❌ אותו דבר +if collection is not None: # ✅ +``` + +הזריקה כאן היא **פיצ'ר**: היא מונעת את הבלבול בין "האובייקט קיים" ל"האוסף אינו ריק". אבל היא מגיעה כחריגה במקום מפתיע, ולרוב בתוך `try` שבולע. + +**האירוע (CodeBot PR #3199):** שמירת סקילים נכשלה בגלל `if collection:`. ובמופע מוקדם יותר (`4e20f7b5`), הבוט עשה `sys.exit(1)` על מסד תקין לחלוטין. + +### כלל +אובייקטים של SDK נבדקים ב-`is None` / `is not None`, לא באמת בוליאנית — גם כשזה נראה מיותר. הצורה הזו נכונה תמיד; הבוליאנית נכונה רק כשה-SDK הסכים לה. ראה גם דפוס 9 בקובץ הזה (walrus + truthy על env var) — אותה משפחה: ‏truthiness שנשענת על הנחה שלא נבדקה. + +### Commits אמיתיים +- CodeBot PR #3199 (יולי 2026); מופע מוקדם `4e20f7b5` (אוגוסט 2025). + +--- + +## הפניות צולבות + +- **CORE U3** — ולידציה של boundary (תיאוריה כללית) +- **R4** — שלמות exception של SDK (הקובץ הזה הוא ה-deep-dive) +- **BY-STACK/state-machine.md** — אי-התאמת Pydantic schema +- **`bugbot-rules/external-input-isinstance.md`**, **`sdk-error-completeness.md`**, **`sdk-lazy-object-not-validated.md`** diff --git a/tests/fixtures/md_corpus/BY-STACK_hebrew-source.md b/tests/fixtures/md_corpus/BY-STACK_hebrew-source.md new file mode 100644 index 000000000..f61e1574f --- /dev/null +++ b/tests/fixtures/md_corpus/BY-STACK_hebrew-source.md @@ -0,0 +1,303 @@ +# עברית בקוד ובנתונים + +**מתי להחיל:** כל פרויקט שלי. ההערות בקוד בעברית, המשתמשים כותבים +בעברית, וחלק מהמחרוזות בקוד בעברית. + +**למה זה מגיע לקובץ נפרד:** זה משטח שרוב הקוד בעולם לא נוגע בו. הכלים +לא נבנו סביבו, והם מפתיעים — לא בהתנהגות שגויה, אלא בהתנהגות נכונה +שלא ציפיתי לה. + +--- + +## H1. תווי BiDi בהערות מפילים לינטרים + +**חומרה:** MEDIUM — מפיל CI, וההודעה לא אומרת מה קרה +**חזר 3 פעמים באותו פרויקט** + +### איך זה נראה + +הערה בעברית שנכתבה במעבד טקסט, הועתקה מדפדפן, או עברה דרך צ'אט — +עלולה לכלול תווי בקרה **בלתי נראים**: + +| תו | שם | +|---|---| +| `U+200F` | RIGHT-TO-LEFT MARK (RLM) | +| `U+200E` | LEFT-TO-RIGHT MARK (LRM) | +| `U+202A`–`U+202E` | embedding / override | +| `U+2066`–`U+2069` | isolate | + +הם לא נראים בשום עורך. הקוד נראה תקין לגמרי. + +### מה קורה + +- `bandit` → `B613` (Trojan Source, CVE-2021-42574) +- `ruff` → `PLE2502` +- CodeFactor → "3 issues" בלי לומר אילו + +הכלים מחמירים כי תו BiDi יכול לגרום לקוד להיראות אחרת ממה שהמהדר +קורא — טכניקת התקפה אמיתית. בהערות בעברית זה תמים לגמרי, אבל הכלי +אינו יכול להבחין, ובצדק. + +### מלכודת ב-debug + +CodeFactor דיווח "3 issues" בלי פירוט. הנחתי `F811` (שהיה קיים בריפו +ממילא) והלכתי לכיוון הלא נכון. **רק הרצת `bandit` ישירות גילתה.** + +כשלינטר מדווח מספר בלי שמות — להריץ את הכלים אחד-אחד, לא לנחש לפי +מה שכבר ידוע על הריפו. + +### התיקון השורשי + +בדיקה שסורקת את כל קבצי ה-`.py`: + +```python +from pathlib import Path + +# ה-escapes מכוונים: התווים עצמם בקובץ הזה היו מפילים את הבדיקה על עצמה +BIDI = ("\u200e", "\u200f", "\u202a", "\u202b", "\u202c", + "\u202d", "\u202e", "\u2066", "\u2067", "\u2068", "\u2069") + +# כל התיקיות שמכילות קוד או תוכן שלנו. ההיקף חייב לכסות את מה שהלינטר +# סורק — אחרת הבדיקה עוברת ו-CI נופל. +ROOTS = (Path("app"), Path("tests"), Path("scripts")) + +def test_no_bidi_controls_in_source(): + """תווי BiDi בלתי נראים בהערות עברית מפילים את bandit ב-B613.""" + for root in ROOTS: + for path in root.rglob("*.py"): + text = path.read_text(encoding="utf-8") + for char in BIDI: + assert char not in text, f"{path}: תו BiDi {char!r}" +``` + +**ההיקף חייב להתאים לזה של הלינטר.** סריקה של `app/` בלבד עוברת בשעה +ש-`bandit` סורק גם את `tests/` ומפיל את ה-CI — בדיוק סוג הפער +ש-`TESTING-PATTERNS.md` T1 מתעד. + +**מלכודת:** הבדיקה נכשלת על עצמה אם התווים כתובים בה כליטרלים. +חייבים escape (`"\u200f"` ולא התו עצמו). + +**למה בדיקה ולא רק תיקון:** תיקנתי פעם אחת, ואז כתבתי עוד הערות +בעברית בשני PRים נוספים והכנסתי אותם מחדש. הבדיקה תפסה את שניהם. + +--- + +## H2. `secrets.compare_digest` נופל על non-ASCII + +**חומרה:** MEDIUM — מפיל את האימות עצמו + +```python +secrets.compare_digest("סוד", user_input) # TypeError +``` + +הפונקציה דורשת מחרוזות ASCII בלבד, או `bytes`. סוד שנכתב בעברית לא +מחזיר `False` — הוא **זורק**. + +### התיקון + +```python +secrets.compare_digest(expected.encode("utf-8"), given.encode("utf-8")) +``` + +### כלל לזיהוי + +כל `compare_digest` על ערך שיכול להגיע מקונפיג, ENV, או DB — לקודד +את שני הצדדים. + +--- + +## H3. כותרות HTTP אינן UTF-8 + +**חומרה:** MEDIUM + +טוקן שמכיל תו עברי או אמוג'י **נשבר בצד הלקוח לפני שהגיע לשרת** — +הדיבאג בשרת לא יראה כלום. + +**הניסוח המדויק:** ערכי כותרת אינם UTF-8, אבל גם אינם "latin-1" סתם. +RFC 9110 מגדיר `field-value` כ-VCHAR של ASCII, ו-`obs-text` שספריות +מפענחות כ-latin-1 מטעמי תאימות היסטורית. **הדרישה האמיתית נגזרת +מדקדוק השדה**, לא מהקידוד: + +| שדה | מה הדקדוק דורש | +|---|---| +| `Authorization: Bearer` | `token68` — ASCII בלבד: `A-Za-z0-9-._~+/` ו-`=` בסוף | +| `Authorization` בסכמות אחרות | תלוי סכמה. `Basic` הוא base64, כלומר ASCII | +| `X-*` משלנו | **אנחנו מגדירים.** אם הוא נושא token — ASCII | +| שדות טקסט חופשי | קידוד לפי RFC 8187 (`field*=UTF-8''...`) | + +### כלל לזיהוי + +ערך שנוסע בכותרת ונושא token או מזהה — הוולידציה בעליית השירות +חייבת לדרוש ASCII **ולומר למה**. הודעה שרק אומרת "לא תקין" תשלח את +המשתמש לחפש במקום הלא נכון. אל תחיל דרישת ASCII גורפת על כל `X-*` +בלי להגדיר קודם מה השדה נושא. + +--- + +## H4. נורמליזציה של עברית בחיפוש + +**חומרה:** LOW — פיצ'ר שנראה שבור + +ניקוד וסופיות משנים את המחרוזת בלי לשנות את המילה. חיפוש שלא מנרמל +לא ימצא מסמך שנכתב עם ניקוד, והמשתמש יסיק שהחיפוש שבור. + +**שני דברים נפרדים, ולא לבלבל ביניהם:** + +- **ניקוד** (`U+0591`–`U+05C7`) — `strip_nikud` מסיר אותו. להפעיל על + השאילתה **ועל** התוכן המאונדקס, אחרת הנרמול חד-כיווני. +- **אותיות סופיות** (`ךםןףץ`) — `strip_nikud` **אינו** נוגע בהן. הן + אות אחרת ב-Unicode, ולכן "ארץ" ו-"ארצות" לא יתאימו בהתאמה מדויקת. + מי שרוצה גם את זה צריך מיפוי מפורש, ורצוי גם stemming. + +לפני שמסתמכים על פונקציה קיימת — לוודא מה בדיוק היא עושה. בדיקה +נכתבת במקום שהפונקציה ממומשת, לא בשכבת החיפוש. + +ל-Postgres full-text: לוודא שה-configuration מטפל בעברית, או לנרמל +לפני האינדוקס. + +--- + +## H5. הודעות שגיאה למשתמש בעברית, ללא פרטים טכניים + +**חומרה:** נגזר מ-`CRITICAL-PATTERNS.md` K6 + +מתועד כבר בספרייה, מוזכר כאן כי הוא נופל באותו משטח: הודעת שגיאה +טכנית באנגלית שדולפת למשתמש הישראלי היא גם חוויה גרועה **וגם** דלף +מידע. הודעה גנרית בעברית למשתמש, הפרטים ללוג. + +--- + +## H6. חיתוך לפי בייטים על תוכן עברי + +**חומרה:** HIGH — הוא **מפיל את השאילתה**, ומה שהמשתמש מקבל תלוי במי שתופס אותה: פולבאק יקר שרץ במקומה, "לא נמצאו תוצאות" על חיפוש שמעולם לא רץ, או 404 על משאב שקיים. כלומר תוצאה שגויה **בלי** שגיאה שמישהו יראה + +### איך זה נראה + +פונקציה או אופרטור שמודדים ב**בייטים** מקבלים אינדקס שחושב ב**תווים** — או פשוט חותכים באמצע אות. באנגלית אות היא בייט אחד, שתי היחידות זהות, והקוד עובר סקירה בלי שאיש ירים גבה. בעברית כל אות היא שני בייטים, ואז החישוב שגוי והחיתוך נוחת באמצע תו. + +מונגו מפרידה בין השניים במפורש, ושמות האופרטורים דומים מספיק כדי להחליף ביניהם בלי לשים לב. נמדד על `"שלום world"`: + +| מה הורץ | התוצאה | היחידה | +|---|---|---| +| `$regexFind ← idx` | `5` | **תווים** | +| `$indexOfCP` | `5` | תווים | +| `$indexOfBytes` | `9` | בייטים | +| `$strLenCP` | `10` | תווים | +| `$strLenBytes` | `14` | בייטים | + +והקוד לקח את `idx` (תווים) והכניס אותו ל-`$substrBytes` (בייטים): + +```python +'_match_idx': {'$ifNull': ['$_m.idx', 0]}, # ← אינדקס תווים +'_snippet_start': {'$max': [0, {'$subtract': ['$_match_idx', 50]}]}, +'snippet_preview': {'$substrBytes': ['$code', '$_snippet_start', 200]}, # ← מצפה לבייטים +``` + +### שתי צורות הכשל + +שתיהן שוחזרו מול MongoDB 8.0.32, ואחת מהן היא בדיוק זו שהופיעה בפרודקשן: + +| מה הורץ | מה מונגו החזירה | +|---|---| +| `$substrBytes("שלום world", 0, 3)` | `ending index is in the middle of a UTF-8 character` (`Location28657`) | +| `$substrBytes("שלום world", 1, 5)` | `starting index is a UTF-8 continuation byte` | + +זה גם מסביר למה הכשל **אינו דטרמיניסטי**: הוא תלוי במקום שאליו נוחת גבול החיתוך, ולא רק בנוכחות עברית בתוכן. לפעמים הגבול נופל בין תווים והכל עובר. + +### למה זה יקר יותר ממה שזה נראה + +הכשל היה בתוך `try/except` שנפל למסלול חלופי. כלומר: + +1. **חיפוש שבור נראה בדיוק כמו חיפוש שלא מצא.** כשגם הפולבאק נופל, המשתמש מקבל "לא נמצאו תוצאות" על שאילתה שמעולם לא רצה. אין שום סימן. +2. **מסלול חלופי יקר רץ לחינם.** במקום הפייפליין המהיר רצה סריקה מלאה בלי אינדקס — שנמדדה כשאילתה האיטית ביותר בלוח. +3. **הסטטיסטיקה מזדהמת.** הפרופיילר רשם את שאילתת הפולבאק כשאילתה איטית אמיתית, ומי שניתח אותה ניתח שאילתה שלא הייתה אמורה לרוץ מלכתחילה. + +ובמופע השני של אותו באג, ה-`except` החזיר **404 "קובץ לא נמצא" על קובץ שקיים**. + +### התיקון השורשי + +לעבור לאופרטורים התו-מבוססים: `$substrCP` במקום `$substrBytes`, `$strLenCP` במקום `$strLenBytes` — אבל **רק במדידות שהן על טקסט**. גודל קובץ נשאר בבייטים, כי הוא באמת בייטים. + +```text +$substrCP("שלום world", 0, 3) ← "שלו" +$substrCP("שלום world", 5, 200) ← "world" (מעבר לסוף — לא זורק) +$substrCP("שלום", 99, 200) ← "" (התחלה מעבר לסוף — לא זורק) +``` + +**הסימן שהתיקון הוא שורש ולא טלאי:** באותו ריפו היו שלושה מסלולים שייצרו את אותו snippet, ושניים מהם כבר חתכו בתווים (`content[start:end]` בפייתון). רק מסלול האגרגציה חרג. כלומר הכוונה בקוד הייתה תווים מלכתחילה, ורק שלב אחד סטה ממנה. + +### כלל לזיהוי + +כל מקום שבו מדידה ומיקום נעשים בשתי יחידות שונות, או שנעשה חיתוך על מחרוזת שיכולה להכיל תווים רב-בייטיים: + +- **מונגו:** `$substrBytes` / `$strLenBytes` / `$indexOfBytes` על שדה טקסט. שים לב במיוחד לערבוב עם `$regexFind ← idx`, שהוא **תווים**. +- **פייתון:** `text.encode()[a:b]`, או תקציב באורך בייטים שנאכף על מחרוזת. ⚠️ כאן החיתוך עצמו **אינו זורק** — הוא מחזיר רצף בייטים פגום, והכשל מגיע מאוחר יותר ב-`decode()` או אצל צרכן שדורש UTF-8, רחוק מהמקום שגרם לו. זה הופך אותו לקשה לאיתור יותר ממונגו, שדוחה את הגבול מיד. +- **SQL:** `LEFT()` / `SUBSTRING()` — ב-MySQL הן על תווים ו-`SUBSTRING(BINARY ...)` על בייטים; ב-Postgres `substr` על תווים ו-`substr` על `bytea` על בייטים. לוודא מול המנוע שבשימוש. +- **תקרת אורך שנשלחת החוצה** (טלגרם, כותרת, שדה DB) — בדוק אם התקרה נמדדת בבייטים או בתווים, כי זה משנה פי שניים בעברית. + +### False positives + +- מדידה שבאמת עוסקת בגודל אחסון או בגודל תעבורה — שם בייטים הם היחידה הנכונה. +- קבצים בינאריים, שאין להם מושג "תו". + +--- + +## H7. גיאומטריה שמניחה LTR, והצמדה שמצמידה מיקום ולא גודל + +**חומרה:** MEDIUM-HIGH — שלושת המופעים הגיעו למשתמש, ואחד מהם **נועל אלמנט מחוץ למסך בלי שום דרך להחזיר אותו** + +### איך זה נראה + +נוסחה שמתרגמת תנועת מצביע למיקום או לגודל, שנכתבה מתוך ההנחה שהציר מתחיל משמאל. בפריסת LTR היא נכונה — ולכן היא עוברת סקירה, ועוברת גם את הבדיקה, כי הבדיקה נכתבה באותה הנחה. זה **אינו** quirk של API (זה R3); כאן החשבון עצמו שגוי. + +```js +// ❌ נוסחת LTR: נכונה רק כשהסיידבר משמאל למפריד +const w = startWidth + (clientX - startX); +// ✅ הסימן נגזר מהפריסה בפועל +const rtl = getComputedStyle(el).direction === 'rtl'; +const w = startWidth + (rtl ? (startX - clientX) : (clientX - startX)); +``` + +**נמדד (CodeBot PR #3286):** האצבע ב-1038 והמפריד ב-1078; האצבע ב-958 והמפריד ב-1138 — פער של 180px, והמפריד זז **הפוך** מהאצבע. + +### הצמדה שמצמידה מיקום ואינה מצמצמת גודל + +```js +const maxLeft = vpW - w; // ❌ כש-w > vpW הביטוי שלילי +el.style.left = clamp(x, minLeft, maxLeft) + 'px'; // וההצמדה עובדת על טווח הפוך +``` + +כשהאלמנט רחב מהמסך, `vpW - w` יוצא **שלילי**, כלומר גבול עליון שקטן מהגבול התחתון. מכאן התוצאה תלויה בצורת ההצמדה בפועל — ‏`Math.min` לבד ידחף למספר שלילי, והצמדה שמיישרת בין שני גבולות סותרים (או שנופלת חזרה על `minLeft`) תשאיר את האלמנט גולש החוצה. ‏**המדידה ב-CodeBot:** על מסך ברוחב 380 הפתק נחת על `x=621`, כלומר לגמרי מחוץ למסך (PR #3299). הנקודה אינה הביטוי המדויק אלא שההצמדה קיבלה טווח בלתי אפשרי — ובכל אחת מהצורות התוצאה היא אלמנט שאי אפשר להגיע אליו. + +**ולמה זו תקיעה ולא אי-נוחות:** ידית הגרירה בכותרת וידית שינוי הגודל בפינה הן הכלים **היחידים** לחלץ את האלמנט — והן בדיוק מה שיצא מהמסך. כל הצמדה לגבולות ה-viewport חייבת לצמצם **גם גודל** (`w = Math.min(w, vpW - margin)`) לפני שהיא מצמידה מיקום. + +### שתי נוסחאות לאותו מימד + +גובה ה-CodeMirror נקבע בשני מקומות בשתי נוסחאות — `max-height: 60vh` מול `calc(100vh - 200px)` — ואף אחת מהן לא הכירה את 106 הפיקסלים שיושבים ביניהן. מתחת לגובה חלון של כ-800px, `overflow: hidden` בלע את העודף **בשקט**, והשורות האחרונות לא היו נגישות בשום גלילה. נמדד: חלון 600px הסתיר 5.4 שורות, 680px הסתיר 2.19 (CodeBot PR #3362). + +זהו R6 (עותק שני של כלל) בלבוש CSS: הכלל "כמה גבוה העורך" נכתב פעמיים, והם נסחפו. + +### כלל לזיהוי + +1. `clientX` / `pageX` / `offsetX` בחיסור, בלי `direction` או `getBoundingClientRect` שקובעים את הסימן. +2. `left` / `right` / `marginLeft` שנקבעים ישירות בפריסה שיכולה להיות RTL — במקום `inset-inline-start` / logical properties. +3. `Math.min(x, vpW - w)` או כל הצמדה ל-viewport, בלי שורה שמצמצמת את `w` עצמו קודם. +4. אותו מימד (גובה, רוחב, ריפוד) שנקבע ביותר ממקום אחד — CSS ו-JS, או שני קבצי CSS. +5. `overflow: hidden` על מכל שגובהו מחושב — הוא הופך שגיאת חישוב לתוכן שנעלם בלי סימן. + +### מצב מומלץ + +**strict** לגרירה, שינוי גודל והצמדה; **warning** למיקום סטטי. והאימות אינו קריאת קוד: מדידה בדפדפן אמיתי **בשתי הפריסות**, עם ריצת בקרה על הקוד שלפני התיקון — שני המספרים, לפני ואחרי. הצורה הזו היא מה שתפס את כל שלושת המופעים. + +### False positives + +- ממשק שמוצהר כ-LTR בלבד (עורך קוד, טרמינל) — גם בעמוד עברי. +- חישוב על קנבס או SVG עם מערכת צירים משלו, שאינה מושפעת מ-`direction`. + +--- + +## ראה גם + +- `bugbot-rules/hebrew-source-and-data.md` +- `docs/source-projects/codebot-patterns.md` Pattern 10 — האירוע המלא של H6 +- `CRITICAL-PATTERNS.md` K6 — secret / stack trace ב-response diff --git a/tests/fixtures/md_corpus/BY-STACK_react-frontend.md b/tests/fixtures/md_corpus/BY-STACK_react-frontend.md new file mode 100644 index 000000000..341810fac --- /dev/null +++ b/tests/fixtures/md_corpus/BY-STACK_react-frontend.md @@ -0,0 +1,204 @@ +# BY-STACK: React frontend + +## רלוונטיות — העתק את הקובץ הזה אם בפרויקט יש... +- ✅ React עם `useState` / `useEffect` / `useCallback` / `useMemo` +- ✅ Data fetching דרך TanStack Query, SWR, או דומה (state של frontend שמסונכרן מהשרת) +- ✅ טפסים, dropdowns, או כל קומפוננטה שמקבלת ערך התחלתי מ-prop שיכול להשתנות +- ✅ צד-דפדפן `URL.createObjectURL`, `setTimeout`, או DOM manipulation +- ⏭ דלג אם: HTML עם server-rendering בלבד (לא SPA), או React עם state מקומי בלבד + +> Cross-link: **K4 (XSS via innerHTML)** ב-`CRITICAL-PATTERNS.md` הוא חובת קריאה לכל פרויקט React שמרנדר מחרוזות חיצוניות. + +--- + +## דפוס 1 — useState מאותחל מ-prop שמשתנה (CORE U2) + +**תדירות:** Noa, EmailFlow, Web — 3+ מופעים +**חומרה:** MEDIUM (שחיתות שקטה כשה-state הישן נשלח חזרה ל-backend) + +### איך זה נראה +```jsx +function StatusDropdown({ leadId, currentStatus }) { + const [status, setStatus] = useState(currentStatus); // ❌ + // currentStatus משתנה כש-parent עושה refetch, אבל status לא + return ; +} +``` +אחרי refetch של הרשימה שמעדכן את ה-status של ה-lead, ה-dropdown עדיין מציג את הערך הישן. המשתמש לוחץ Save, ה-dropdown שולח את ה-`expected_status` הישן, בדיקת CAS אופטימית ב-backend דוחה, ה-pipeline חוזר אחורה. + +### שלושה דפוסי resync (לבחור אחד לכל הפרויקט) +1. **`key` prop על parent (הכי פשוט):** + ```jsx + + ``` + מאלץ את React לעשות remount לקומפוננטה ב-`id` change, כך שה-state המקומי טרי. +2. **`useEffect` resync:** + ```jsx + const [status, setStatus] = useState(currentStatus); + useEffect(() => { setStatus(currentStatus); }, [currentStatus]); + ``` +3. **Derived state (בלי state מקומי):** + ```jsx + // רנדר ישירות מ-currentStatus, הרם edits ל-parent + return ; + ``` + +### כלל לזיהוי +דווח על `const [s, setS] = useState(props.X)` כש-`props.X` יכול להשתנות וגם אין `useEffect([props.X])` resync וגם אין `key={...}` על הקומפוננטה. + +### False positives +- props רק ב-mount (`user.id` שלעולם לא משתנה). +- state מקומי בלבד (modal פתוח/סגור). +- inputs uncontrolled עם `defaultValue`. + +### Commits אמיתיים +- EmailFlow `d84daca`, `3987818` — status dropdown תקוע על ערך ישן. +- EmailFlow `02f633a` — חסר `key={conversationId}` ב-ReplyBox → state עבר בין שיחות. +- EmailFlow `b14e3f3` — `seenDraftKey` collision בין שתי הודעות. +- Noa `TBD-followup-rules` — `FollowupRulesSection` לא סינכרן state אחרי save (prop השתנה, useState המקומי תקוע). +- Noa `TBD-use-local-storage` — `useLocalStorage` hook לא איפס state ל-`defaultValue` כש-ה-key prop השתנה ולא היה ערך שמור. +- Web `2e5c480` — רק `token` עודכן, לא `refreshToken`. + +--- + +## דפוס 2 — הפרת Rules of Hooks + +### איך זה נראה +```jsx +function Comp({ data }) { + if (!data) return null; + const [x, setX] = useState(data.x); // ❌ hook אחרי early return + ... +} +``` +ספירת hooks משתנה בין renders → React קורס ("Rendered fewer hooks than expected"). + +### כלל לזיהוי +כל `useState` / `useEffect` / `useCallback` / `useMemo` חייב לרוץ לפני כל `return null;` / conditional return. + +### Commits אמיתיים +- EmailFlow `e968135`. + +--- + +## דפוס 3 — Stale closure ב-useCallback / useMemo + +### איך זה נראה +```jsx +const toggleActivity = useCallback((id) => { + api.toggle(activeChildId, id); // ❌ activeChildId לא ב-deps +}, []); +``` +אחרי החלפת ילד, ה-callback עדיין משתמש ב-`activeChildId` הקודם. + +### כלל לזיהוי +הפניות בגוף `useCallback` / `useMemo` חייבות כולן להופיע ב-dependency array. ה-`react-hooks/exhaustive-deps` של ESLint תופס את רובן — שמור עליו ב-`error`. + +### Commits אמיתיים +- routine `259c2a3` — `activeChildId` חסר → toggle השפיע על הילד הקודם. + +--- + +## דפוס 4 — setState אחרי `await` בלי cancellation flag + +### איך זה נראה +```jsx +useEffect(() => { + let cancelled = false; + async function load() { + const data = await fetchData(); + if (cancelled) return; + setData(data); + } + load(); + return () => { cancelled = true; }; +}, [...]); +``` +בלי בדיקת `cancelled`, ה-`await` של ה-render השני מסיים ודורס את ה-UI של הראשון. + +### Commits אמיתיים +- Noa `b360c66` — `renderTemplate` async דרס את ה-UI עם preview ישן. +- Noa `7444dd9` — `Promise.all` setLoading התחרה עם render. + +--- + +## דפוס 5 — Identity לא יציב ב-deps (אובייקטים של TanStack Query mutation) + +```jsx +const mutation = useMutation(...); +useEffect(() => { mutation.mutate(...); }, [mutation]); // ❌ identity של mutation לא יציב → re-run אינסופי +``` +אובייקט ה-mutation של TanStack Query נוצר מחדש בכל render. השתמש ב-`mutation.mutate` (identity יציב של callback) ב-dep array, או `useEffect(() => { ... }, [])` עם `mutation.mutate` בגישה מבפנים (eslint suppression עם הערה). + +--- + +## דפוס 6 — דליפת `URL.createObjectURL` + +```jsx +const url = URL.createObjectURL(blob); // ❌ לעולם לא משוחרר +``` + +```jsx +useEffect(() => { + const url = URL.createObjectURL(blob); + setSrc(url); + return () => URL.revokeObjectURL(url); +}, [blob]); +``` + +### Commits אמיתיים +- Web `f5cbaf9` — דליפת blob URL של תמונת profile. + +--- + +## דפוס 7 — `setTimeout(fn, delay)` / `new Date(value)` עם delay לא סופי + +```js +setTimeout(load, parsed); // ❌ אם parsed הוא NaN, שגיאת React +setTimeout(load, isFinite(parsed) && parsed > 0 ? parsed : 1000); // ✅ +``` + +### Commits אמיתיים +- EmailFlow `e432866` — `setTimeout(fn, NaN)` קרס. + +--- + +## דפוס 8 — CSS reset גלובלי שדורס מחלקות utility + +```css +* { margin: 0; padding: 0; } /* ❌ דורס Tailwind space-y-*, p-*, m-* */ +``` + +השתמש ב-reset מוגבל-scope (למשל `@tailwind base` בלבד) או הסר את ה-selector של `*`. + +### Commits אמיתיים +- Web `c586691`. + +--- + +## דפוס 9 — Tooltip / dropdown נסגר על click בגלל bubbling של event + +```jsx +
+ ... {/* clicks בפנים בועלים למעלה → תפריט נסגר */} +
+``` + +עצור propagation בתוך האלמנט הפנימי: +```jsx + e.stopPropagation()}>... +``` + +### Commits אמיתיים +- Markdown-Academy `315154e`. + +--- + +## הפניות צולבות + +- **CORE U2** — סנכרון state ב-React (הקובץ הזה הוא ה-deep-dive) +- **CORE U1** — חל כש-React קורא ל-endpoints שמקושרים ל-webhook (idempotency keys ב-request) +- **CRITICAL K4** — XSS via `innerHTML` / `dangerouslySetInnerHTML` +- **R3** — quirks רחבים יותר של Browser API (clipboard, mailto:, blob URLs) +- **`bugbot-rules/react-stale-state-on-prop.md`** — prompt עצמאי +- **`bugbot-rules/react-async-media-resource.md`** — משאב async חיצוני (מיקרופון, WebSocket, MediaRecorder): double-action, setState אחרי unmount, ו-cleanup בכל מסלולי היציאה diff --git a/tests/fixtures/md_corpus/CORE-PATTERNS.md b/tests/fixtures/md_corpus/CORE-PATTERNS.md new file mode 100644 index 000000000..0ced02018 --- /dev/null +++ b/tests/fixtures/md_corpus/CORE-PATTERNS.md @@ -0,0 +1,294 @@ +# דפוסים מרכזיים (CORE) + +דפוסי באגים אוניברסליים — אלה מופיעים ב**כל שלושת** מסמכי המקור (Noa_Leads, EmailFlow, וסט 8-הפרויקטים) על stacks שונים, פרויקטים שונים וטווחי זמן שונים. הם משקפים הרגלים אישיים וחוסרי שיקול דעת, לא בעיות ספציפיות לפרויקט. **החל אותם על כל פרויקט חדש, ללא קשר ל-stack.** + +מוסכמת הספירה: `X/3 מקורות` מתייחסת תמיד לשלושת מסמכי ההצלבה המקוריים. מסמכי מקור מאוחרים (Markdown-Docs, CodeBot) שמוסיפים ראיות לדפוס קיים מסומנים בשורת התדירות כתוספת מפורשת — הם מחזקים את הדפוס אבל לא משנים את הקיבוץ ל-tiers. + +לכל דפוס: commits אמיתיים מצוטטים, כלל זיהוי גנרי, false positives, ומצב אכיפה מומלץ. + +--- + +## U1. Race conditions / async TOCTOU + +**תדירות:** 3/3 מקורות, ~15 מופעים +**פרויקטים:** Noa_Leads, EmailFlow, Shipment-bot, Facebook-Leads-New, Markdown-Academy, routine +**חומרה:** HIGH + +### איך זה נראה +נתיבי קוד concurrent קוראים מצב משותף, מסתעפים לפיו, ואז כותבים — בלי atomicity. מתבטא כ: +- כפילויות מ-webhooks / queue workers / cron + beat scheduler מקבילים. +- `await` חסר שגורם ל-coroutines להיות truthy תמיד. +- בדיקת lock שמתבצעת *מחוץ* ל-lock שהיא אמורה לשמור. +- Cursors / CAS שמשתמשים ב-`=` במקום `IS NULL` על עמודות nullable. + +### וריאציות של הדפוס +אותו root cause, מופעים שונים. כולם נפתרים על ידי UNIQUE constraint + transaction lock (או CAS), אבל זיהוי הוריאציה הספציפית עוזר ב-debug: + +- **(a) שורת DB לא נוצרת לפני external call** — הקוד קורא ל-`await client.X()` (Gmail draft, SMS, payment, webhook emit) *לפני* הכנסת שורת "intent" מקומית עם UNIQUE constraint. at-least-once delivery (Pub/Sub, retries, parallel workers) יוצרת orphans בצד החיצוני. +- **(b) פער check-then-act בין קריאה לכתיבה** — הקוד עושה `SELECT ... WHERE status='pending'`, מסתעף, ואז `UPDATE ... WHERE id=:id`. שני runners עוברים את הקריאה, שניהם מריצים את ה-update, שניהם מבצעים את הפעולה. +- **(c) Cursor / CAS column לא מתקדם atomically** — webhook קורא `sync_token`, קורא ל-API חיצוני, וכותב חזרה `next_sync_token` ב-statement נפרד. שני webhooks מתחרים על ה-cursor; שניהם מחילים את אותם deltas. +- **(d) `await` חסר על predicate async** — פונקציה async נקראת בהקשר בוליאני (`if foo():`); ה-coroutine תמיד truthy, אז ה-gate אף פעם לא חוסם. + +ב-debug — זהה קודם את הוריאציה. התיקון מאותה משפחה (CAS / UNIQUE / lock), אבל המיקום הנכון שונה. + +### דוגמאות אמיתיות +- **Noa (`33af59e`):** שני webhooks מקבילים של Google Calendar קראו את אותו `sync_token`, החילו deltas → שורות Activity כפולות. תיקון: optimistic lock + CAS (`WHERE history_id = expected_old`). +- **Noa (`cf99698`):** CAS על `WHERE history_id = expected_old` לא טיפל ב-`NULL` ב-Postgres → cursor תקוע לנצח. +- **EmailFlow (`f847a44`):** תשעה Pub/Sub workers מקבילים יצרו Gmail draft לפני שאחד הספיק לעשות commit → 9 drafts יתומים. תיקון: INSERT מקומי עם `UNIQUE` *לפני* הקריאה ל-Gmail. +- **Shipment-bot (`457eea1`):** `SELECT ... WHERE status='PENDING'` ואז `UPDATE`; ה-beat scheduler ו-`send_message.delay()` שניהם בחרו את השורה → ה-OTP נשלח פעמיים. תיקון: atomic `UPDATE ... WHERE status='PENDING' RETURNING id`, בדיקת rowcount. +- **Shipment-bot (`f1e0fbb`):** `_is_ip_blocked()` הפך ל-async, אבל הקורא לא הוסיף `await` → ה-coroutine תמיד truthy → כל webhook החזיר 429. +- **Facebook-Leads-New (`5823724`):** `_check_daily_limit()` רץ מחוץ ל-`scan_lock` → שתי קריאות מקבילות עברו את הבדיקה, שתיהן הריצו scans. +- **Markdown-Academy (`7c955f1`):** עליית השרת קראה ל-seed כמה פעמים במקביל בלי הגנה → שיעורים נוצרו פעמיים. + +### כלל לזיהוי (להעתקה ל-CLAUDE.md / bugbot) +דווח על כל אחד מהבאים: +1. `await session.commit()` ולאחריו `await client.X()` (קריאה חיצונית בלתי הפיכה) בלי INSERT מקדים עם UNIQUE constraint או row lock. +2. `SELECT` / ORM `.scalar()` שמחזיר state, ואז סעיף, ואז `UPDATE` על אותה שורה בלי `WHERE` על הערך שנקרא (אין CAS). +3. פונקציית `async def` שנקראת בלי `await` ושימוש בה בהקשר בוליאני (`if foo():`). +4. critical section מוגן ב-lock שבו precondition נבדק *לפני* רכישת ה-lock. +5. CAS / cursor שמושווה עם `=` כשהערך יכול להיות `NULL` (Postgres: `col = NULL` הוא `NULL`, לא `TRUE` — חייב להסתעף עם `IS NULL`). + +### False positives +- קריאות חיצוניות read-only (status fetch, analytics fire-and-forget) — אין צורך ב-reserve. +- script של תהליך יחיד בלי runners concurrent — race תיאורטי. +- כתיבה משתמשת ב-`INSERT ... ON CONFLICT` או UNIQUE שכבר תופס כפילויות. +- הקריאה מתבצעת בתוך בלוק `SELECT FOR UPDATE`. + +### מצב מומלץ +**strict** ל-webhook handlers, queue/Celery tasks, cron jobs, payment / messaging / OTP flows. +**warning** לכלי admin פנימיים ו-scripts בריצה יחידה. + +### ראה גם +- `BY-STACK/webhooks.md` — ספציפיות Pub/Sub at-least-once, signature, idempotency +- `BY-STACK/async-orm.md` — דפוסי CAS ב-SQLAlchemy, advisory locks +- `BY-STACK/cron-jobs.md` — race conditions בצד ה-cron +- `bugbot-rules/race-toctou.md` — prompt עצמאי + +--- + +## U2. סנכרון state ב-React / stale closure + +**תדירות:** 3/3 מקורות, ~6 מופעים +**פרויקטים:** Noa_Leads (frontend), EmailFlow (frontend), routine, Web +**חומרה:** MEDIUM (שובר UX; data corruption כשה-status נשלח חזרה ל-backend) + +### איך זה נראה +`useState` מקומי שמאותחל מ-prop שמשתנה, ולא מסונכרן מחדש; או callback שתופס משתנה ישן; או hooks שנקראים אחרי early return ושוברים את Rules of Hooks. + +### דוגמאות אמיתיות +- **Noa (`3fc0ec6`):** `useEffect` עם dep על reference של אובייקט הופעל מחדש ודרס edits של המשתמש. +- **Noa (`b360c66`):** `renderTemplate` async ללא בדיקת cancelled flag → preview ישן דרס את ה-UI. +- **EmailFlow (`e968135`):** `useState` אחרי `return` מוקדם → ספירת hooks משתנה → React crash. +- **EmailFlow (`02f633a`):** חסר `key={conversationId}` ב-`ReplyBox` → state עבר משיחה לשיחה. +- **EmailFlow (`d84daca` + `3987818`):** dropdown של status סונכרן רק על `leadId`; refetch של רשימה עדכן status ב-DB אבל ה-dropdown נשאר על ערך ישן → save שלח `expected_status` שגוי → pipeline חזר אחורה. +- **routine (`259c2a3`):** `activeChildId` חסר מ-deps של `useCallback` → אחרי החלפת ילד, פעולה השפיעה על הילד הקודם. +- **Web (`2e5c480`):** אחרי email verification רק `token` עודכן ב-`localStorage`; `refreshToken` נשאר ישן → reauth נשבר. + +### כלל לזיהוי +דווח על כל אחד מהבאים: +1. `const [s, setS] = useState(props.X)` כש-`props.X` יכול להשתנות, וגם אין `useEffect([props.X])` resync וגם אין `key={...}` על הקומפוננטה. +2. `useState` / `useEffect` / `useCallback` / `useMemo` שנקראים אחרי `if (...) return null;` או כל early return. +3. `useCallback` / `useMemo` שגוף הפונקציה שלהם מתייחס ל-prop או state שלא נמצא ב-dependency array. +4. עדכון חלקי של ערך multi-field (למשל עדכון `token` בלי `refreshToken`). +5. `setState` אחרי `await` בלי בדיקת cancellation flag. + +### False positives +- state מקומי בלבד (modal פתוח/סגור, theme toggle) — לא תלוי בנתונים חיצוניים. +- inputs uncontrolled עם `defaultValue` — מכוון. +- קומפוננטה שמקבלת prop פעם אחת ב-mount (למשל `user.id`) — אין צורך ב-sync. +- ה-dep array מדלג בכוונה על identity לא יציב (למשל אובייקט mutation של TanStack) — כדאי שתהיה הערת תיעוד מסבירה. + +### מצב מומלץ +**warning** (strict מייצר רעש גבוה על state מקומי לגיטימי). + +### ראה גם +- `BY-STACK/react-frontend.md` — deep-dive עם שלושת דפוסי ה-resync +- `bugbot-rules/react-stale-state-on-prop.md` + +--- + +## U3. ולידציה של external input / boundary + +**תדירות:** 3/3 מקורות + Markdown-Docs (מקור מאוחר), ~13 מופעים +**פרויקטים:** Noa_Leads, EmailFlow, Shipment-bot, Facebook-Leads-New, routine, Markdown-Docs +**חומרה:** MEDIUM (קריסות בטראפיק אמיתי) / HIGH (כשמשולב עם SQL או eval) + +### איך זה נראה +הקוד מניח שקלט חיצוני (תשובות API, payloads של webhook, output של AI, משתני סביבה) במבנה הצפוי — אין `isinstance` guard, אין בדיקת NaN/Inf, regex תופס יותר מדי, subclass של exception ב-SDK לא נתפס. + +### דוגמאות אמיתיות +- **Noa (`c128115`):** `_complete` תפס רק `RateLimitError` + `_RETRYABLE`; subtypes אחרים של `anthropic.APIError` (NotFound, BadRequest, Auth) עברו בלי טיפול. +- **Noa (`95dcce6`):** domain blacklist השתמש ב-exact match על host של email → `mail.mailchimp.com` לא נחסם. +- **Noa (`f27adc1`):** regex חמדן `\{.*\}` על תגובת JSON של AI נשבר על `}` סוגר ב-prose. +- **Noa (`7001892`):** `LeadDraft.service_category` כ-`StrEnum` דחה ערך לא מוכר → אובדן full_name + phone באותו שדה. +- **EmailFlow (`6b7dbeb`):** `headers["list-unsubscribe"].strip()` קרס כשהערך לא היה str (MIME מקולקל). +- **EmailFlow (`46d05f7`):** `parsed_json.get(...)` קרס כי `parsed_json` היה רשימה, לא dict. +- **EmailFlow (`55b4328`):** `parse_webhook` קיבל `messages: None` במקום רשימה. +- **EmailFlow (`e432866`):** `setTimeout(fn, delay)` עם NaN → React crash. +- **Shipment-bot (`97bf0bc`):** `AmountValidator` לא בדק NaN/Inf; `NaN < 0` הוא `False`, `NaN > max` הוא `False` → NaN נכנס לארנק כסכום. +- **Facebook-Leads-New (`fadc0dc`):** `\b` ב-Python triple-quoted string הוא backspace, לא word boundary → ה-regex לא רץ. +- **routine (`e5c26ad` / `2571c91`):** מפתחות VAPID פגומים / חסר prefix של `mailto:` → exception לא נתפס ב-`setVapidDetails` → השרת קורס בעלייה. +- **Markdown-Docs (P5):** `form-action` נבנה מ-`urlsplit(redirect_uri).netloc` — שכולל userinfo, שאינו `host-source` חוקי ב-CSP. דפדפן שפוסל את ההנחיה חוסם את הטופס בשקט. גם התיקון הראשון (לחתוך userinfo) נפל — IPv6 בסוגריים גם הוא מחוץ לדקדוק. הפתרון: אימות allowlist מול הדקדוק עצמו. + +### כלל לזיהוי +לפני כל `.get()`, `.append()`, `.strip()`, `[key]`, או iteration על ערך חיצוני, חייב: +1. `isinstance(obj, dict)` אם מצפים ל-dict. +2. `isinstance(items, list)` אם מצפים לרשימה. +3. `isinstance(value, str)` לפני `.strip() / .lower() / .split()`. +4. למספרים: `isinstance(n, (int, float))` + `math.isfinite(n)` + בדיקת טווח. +5. ל-regex על טקסט חיצוני: raw strings (`r"..."`) ו-word boundaries; עדיף `json.JSONDecoder().raw_decode()` על regex ל-JSON משובץ. +6. לקריאות SDK: לתפוס את ה-base class של ה-SDK (`anthropic.APIError`, `googleapiclient.errors.HttpError`), לסדר את ה-`except` subclass-לפני-superclass. +7. לאתחול SDK בזמן startup (VAPID keys, env vars, OAuth secrets): לעטוף את האתחול ב-try/except + לוודא פורמט ב-boot. +8. להזרקת ערך חיצוני למחרוזת עם דקדוק ללא הפרדת נתונים/קוד (CSP, כותרות HTTP, שמות קבצים): שתי בדיקות, לא אחת. (א) תחביר — הערך ניתן לביטוי בדקדוק היעד, ב-`fullmatch` מול הדקדוק עצמו ולא בניקוי תווים אסורים (blocklist לעולם אינו שלם, וערך פגום בהנחיית מדיניות גרוע מערך חסר). (ב) סמנטיקה — הערך גם מורשה: מופיע ב-allowlist של origins/סכמות/ספריות מותרות. `evil.com` הוא host-source תקין תחבירית — רק הבדיקה השנייה עוצרת אותו. לא חל על SQL/shell — שם קיימת הפרדה אמיתית (parameterized queries, `shell=False`) וולידציה אינה תחליף לה. + +9. לבדיקת **שייכות** על ערך חיצוני (`value in `): בדיקת טיפוס **לפני** בדיקת החברות. `in` על קבוצה או מילון מחשב `hash(value)`, וערך לא-hashable זורק `TypeError` במקום להחזיר `False` — כלומר הוולידציה קורסת לפני שהספיקה לומר מה לא תקין, והמשתמש מקבל 500 במקום 400. השורה נראית תמימה בדיוק מפני שהיא נראית כמו שאלה שמחזירה כן/לא. + +### False positives +- מבני נתונים פנימיים שנוצרו על ידי הקוד שלנו (קלט FastAPI שעבר ולידציית Pydantic כבר נבדק ב-isinstance). +- נתיבי `logger.debug` שלא ירוצו בפרודקשן. +- `except` צר ב-propagation מכוון (למשל `AppException` שמגיע ל-FastAPI). + +### מצב מומלץ +**warning** במשך שבועיים על קוד קיים, ואז **strict** לקוד חדש בגבולות מודולים. + +### ראה גם +- `BY-STACK/external-sdk.md` +- `bugbot-rules/external-input-isinstance.md` +- `bugbot-rules/sdk-error-completeness.md` +- `bugbot-rules/blanket-policy-silent-block.md` §3–4 — זווית הדקדוק (P5) +- `docs/source-projects/markdown-docs-mcp-patterns.md` P5 — ההיגיון המלא, כולל למה לא חותכים userinfo + +--- + +## U4. SQL / Postgres edge cases + +**תדירות:** 3/3 מקורות, ~8 מופעים +**פרויקטים:** Noa_Leads, EmailFlow, Shipment-bot, Facebook-Leads-New, routine +**חומרה:** MEDIUM–HIGH + +### איך זה נראה +סמנטיקה של Postgres / SQLAlchemy / SQL כללי מפתיעה את המפתח: +- `col = NULL` מחזיר `NULL`, לא `TRUE`. +- `VARCHAR(N)` קטן מדי לערך enum חדש → INSERT נכשל בפרודקשן, לא ב-SQLite של dev. +- עמודת `Integer` קטנה מדי ל-IDs ממערכות חיצוניות (Telegram, Stripe). +- `ORDER BY` על עמודה לא ייחודית = סדר לא מוגדר ב-ties → דפדוף מדלג/מכפיל. +- `LIKE` מתאים `_` ו-`%` כ-wildcards ב-prefix שהמשתמש סיפק. +- `postgresql_ops` ב-SQLAlchemy בשימוש שגוי ככיוון מיון. +- `ANY(:ids)` עם מערך מחרוזות על עמודת UUID נכשל בלי cast. + +### דוגמאות אמיתיות +- **Noa (`cf99698`):** CAS `WHERE col = NULL` לא תופס NULL — צריך הסתעפות `IS NULL`. +- **Noa (`2c8263a`):** `VARCHAR(20)` קטן מדי לערך חדש של `StrEnum` → INSERT נכשל. +- **Noa (`98e8d18`):** `postgresql_ops={"col": "DESC"}` שגוי (זה למחלקות אופרטור); להשתמש ב-`desc("col")`. +- **Noa (`244286d`):** `ANY(:ids)` עם מערך מחרוזות על עמודת UUID נכשל בלי cast. +- **EmailFlow (`d84daca`):** `Lead.order_by(updated_at.desc())` בלבד — leads עם אותו timestamp קיבלו סדר אקראי → דפדוף בלולאה. +- **EmailFlow (`dfdf975`):** `.strip()` על Column expression לא רץ ב-SQL; היה צריך `func.trim(...)`. +- **Shipment-bot (`b16b99f`):** `entity_id` כ-`Integer`; IDs של Telegram חורגים מ-2³¹ → INSERT נכשל. +- **Shipment-bot (`c0c1b74`):** דפדוף audit log מוין רק לפי timestamp → שורות התערבבו בין דפים. +- **Facebook-Leads-New (`2f45eca`):** `LIKE 'test_key%'` תפס `testXkey` כי `_` הוא wildcard. + +### כלל לזיהוי +1. ב-CAS / `UPDATE ... WHERE col = :val` כש-`col` nullable: הסתעפות `is None` → להשתמש ב-`col.is_(None)`. +2. כל עמודת `String(N)` / `VARCHAR(N)` שמתמלאת מ-enum: לאכוף `N >= max(len(v) for v in EnumClass)` (בדיקת CI או טסט). +3. IDs של Telegram / חיצוניים → `BigInteger` (`BIGINT`). +4. כל `ORDER BY` שאחריו `LIMIT`/`OFFSET` או cursor pagination חייב tiebreaker שני, בדרך כלל המפתח הראשי (`.id`). +5. כל `LIKE` עם prefix מהמשתמש חייב לברוח מ-`_` ו-`%`, או להשתמש ב-`=` מדויק. +6. פעולות מחרוזת של Python (`.strip()`, `.lower()`) על Column expressions → להשתמש ב-`func.trim()`, `func.lower()`. + +### False positives +- אגרגציות (`GROUP BY` + `SUM`) — אין צורך ב-tiebreaker לדפדוף. +- `.first()` של שורה יחידה על עמודה ייחודית. +- עדכוני ORM דרך `session.merge` — לא raw SQL. + +### מצב מומלץ +**strict** ל-nullability, escape של `LIKE`, ובדיקות גודל עמודות. +**warning** ל-tiebreaker חסר בדפדוף ב-queries לא מדפדפים. + +### ראה גם +- `BY-STACK/postgres.md` — כיסוי מלא של SQL/Alembic/דפדוף +- `bugbot-rules/postgres-null-cas.md`, `pagination-tiebreaker.md`, `like-wildcard-injection.md` + +--- + +## U5. עדכוני atomic חלקיים / סטייה ב-linked fields + +**תדירות:** 3/3 מקורות, ~10 מופעים +**פרויקטים:** Noa_Leads, EmailFlow, Shipment-bot, routine, Web +**חומרה:** HIGH (שחיתות נתונים שקטה + צרכנים downstream תלויים ב-cascade) + +### איך זה נראה +פעולה לוגית דורשת עדכון של N שדות מקושרים atomically. הקוד מעדכן `N-1` ושוכח את השאר. השדה ה"שכוח" נקרא מאוחר יותר על ידי cron, dashboard, או flow אחר → אי-עקביות שקטה. + +### דוגמאות אמיתיות +- **Noa (`bd2b105`):** chip קבע `PROPOSAL_SENT` ועקף את ה-`ProposalSentConfirmModal` flow. +- **Noa (`75b430a`):** chip קבע `PROPOSAL_SENT` בלי לכתוב `proposal_sent_at` → `check_stuck_proposals` נשבר. +- **Noa (`df530f3`):** `apply_chip` עדכן status אבל שכח `last_outbound_at` + `last_activity_type`. +- **Noa (`4cc5a09`):** `apply_chip` לא סגר tasks ישנים ב-`AUTO_CLOSE_TASK_TYPES` + אין dedup. +- **Noa (`04cb101`):** `_apply_reschedule` עם `rowcount=0` השאיר booking בלי activity log → cron התייחס לזה כאילו לא קרה. +- **EmailFlow (`f847a44`):** Pub/Sub draft נוצר חיצונית לפני commit מקומי → orphans בכשל חלקי. +- **EmailFlow (`75c0a47`):** audit row נכתב לפני `client.send_draft()` — אם השליחה נכשלה, ה-compliance trail שיקר. +- **routine (`01c61ac`):** `handleMorningTimeChange` שלח רק `hour`, לא את ה-`enabled` flag; בקשות מקבילות דרסו זו את זו. +- **Web (`2e5c480`):** רק `token` עודכן ב-`localStorage`; `refreshToken` נשאר מההרשמה → reauth נשבר. + +### כלל לזיהוי +לכל פונקציה שמעדכנת status / "phase" / lifecycle column של ישות, ודא שגם **רשימת השדות האחים הקנונית** מתעדכנת באותה טרנזקציה. דוגמאות ספציפיות לפרויקט: +- שינוי status → `last_outbound_at`, `last_activity_type`, `proposal_sent_at`, סגירת `AUTO_CLOSE_TASK_TYPES`, log activity, ביטול cache. +- קריאה חיצונית → או INSERT מקומי קודם עם `UNIQUE` *לפני* הקריאה, או כתיבת audit log עם `metadata.applied=false` בכשל. +- רענון token → גם `access` וגם `refresh` בכתיבה atomic אחת (`localStorage.setItem` הוא per-key; השתמש ב-JSON blob אחד או בשתי כתיבות בטרנזקציה). +- submit חלקי של טופס → שלח אובייקט מלא או השתמש ב-server-side merge עם field mask מפורש. + +### False positives +- migrations / scripts של backfill (one-shot, לא touchpoints טרנזקציוניים). +- נתיבים read-only. +- עדכוני שדה יחיד שבאמת עצמאיים (למשל `last_viewed_at` לא קשור לכלום). + +### מצב מומלץ +**strict** בתוך `apply_chip` / `mark_*_sent` / נקודות כניסה של state-machine. +**warning** במקומות אחרים. + +### ראה גם +- `BY-STACK/state-machine.md` — checklist של שלמות touchpoint +- `BY-STACK/webhooks.md` — דפוס reserve-then-fill +- `bugbot-rules/linked-field-atomicity.md` + +--- + +## U6. סטייה בין migration ל-schema + +**תדירות:** 3/3 מקורות, ~6 מופעים +**פרויקטים:** Noa_Leads, EmailFlow, Shipment-bot, Markdown-Academy, routine +**חומרה:** MEDIUM (שובר deploys טריים; שורד ב-CI אם הטסטים משתמשים רק ב-DB ממוגרר) + +### איך זה נראה +שינויי schema חיים בשני מקומות: migration של Alembic / Knex וגם `__table_args__` של מודל SQLAlchemy / ORM. שני המקומות נסחפים; DB טרי (test, dev, prod-from-scratch) מסתיים עם schema שונה מ-DB של פרודקשן אחרי migrations הדרגתיים. + +### דוגמאות אמיתיות +- **Noa (`2c8263a`):** migration הוסיף ערך enum חדש אבל העמודה `VARCHAR(20)` קצרה מדי. +- **EmailFlow (`40f855d`):** FTS index רק ב-migration, לא ב-`__table_args__` של המודל → DB טרי בלי index. +- **EmailFlow (`a3a3134`):** revision id של Alembic ארוך יותר מ-`VARCHAR(32)` של `alembic_version.version_num` → migration נכשל ב-deploy טרי של Render. +- **EmailFlow (`f143e1c`):** migration עם `DROP COLUMN` אבל הקוד עדיין משתמש בה. +- **EmailFlow (`3f43d91`):** `CHECK` constraint ממוין שונה ב-migration לעומת המודל. +- **routine (`80c1fc4`):** migration השתמש ב-`ADD COLUMN IF NOT EXISTS` — לא נתמך ב-MySQL. +- **routine (`7a4e879`):** migration רץ `ALTER TABLE` על טבלה לא קיימת ב-deploy ראשון. +- **routine (`230e0c1`):** קובץ migration כפול + חסר SSL config. +- **Shipment-bot (`b16b99f`):** אי-התאמת טיפוס עמודה (`Integer` קטן מדי ל-Telegram IDs). + +### כלל לזיהוי +1. כל `Index(...)`, `CheckConstraint(...)`, `UniqueConstraint(...)` ב-migration חייב להופיע זהה ב-`__table_args__` של המודל. +2. `DROP COLUMN` ב-PR שעוד יש בו הפניות לעמודה במקומות אחרים → דווח. +3. אורך revision id של Alembic ≤ 32. +4. פרויקטי MySQL: בלי `IF NOT EXISTS` ב-`ADD COLUMN` (syntax של PG בלבד); בלי `ADD COLUMN IF NOT EXISTS`. +5. עמודות `String(N)` / `VARCHAR(N)` שמתמלאות מ-enum → טסט CI שבודק `N >= max(len(value) for value in Enum)`. +6. migrations הם additive כשהקוד הישן עדיין משתמש בעמודה — שינוי הרסני בא ב-migration *נפרד* מאוחר יותר אחרי שהקוד הוסר. + +### False positives +- migrations של data בלבד (backfill, fixup) — אין שינוי schema. +- test fixtures עם `extend_existing=True`. + +### מצב מומלץ +**strict** להתאמה בין migration ל-model. +**warning** לאורך revision id ול-syntax של MySQL (CI תופס בכל מקרה). + +### ראה גם +- `BY-STACK/postgres.md` — כיסוי מלא של migrations +- `bugbot-rules/migration-model-drift.md` diff --git a/tests/fixtures/md_corpus/CRITICAL-PATTERNS.md b/tests/fixtures/md_corpus/CRITICAL-PATTERNS.md new file mode 100644 index 000000000..78e32f661 --- /dev/null +++ b/tests/fixtures/md_corpus/CRITICAL-PATTERNS.md @@ -0,0 +1,573 @@ +# דפוסים קריטיים (CRITICAL) + +דפוסי באגים בחומרה גבוהה — **תמיד החל, ללא קשר ל-stack או לתדירות במסמכי המקור.** חומרה אבטחתית, אובדן נתונים, פרטיות, פיננסי, או זמינות קטסטרופלית. + +חלק מהם הופיעו רק במסמך מקור אחד (סריקת 8-הפרויקטים, שבה עבודת אבטחה נסקרה באופן שיטתי). **החומרה שלהם מצדיקה הצבה בדרג העליון גם כשהתדירות צרה**. כל אחד מתויג עם בסיס הראיות שלו. + +--- + +## K1. השתלטות על חשבון OAuth דרך flow של signup + +**מקור:** 8-Projects C1 (routine — commit `8a39df6`) +**חומרה:** CRITICAL — השתלטות מלאה על חשבון + +### איך זה נראה +flow של signup מאפשר קביעת סיסמה לחשבון שכבר קיים דרך OAuth, ודורש רק את כתובת ה-email כקלט. תוקף שיודע את ה-email של משתמש OAuth יכול: +1. לשלוח signup עם ה-email הזה + סיסמה חדשה. +2. להתחבר עם email/password מאז ואילך, ולעקוף את OAuth. + +### כלל לזיהוי +לכל endpoint של "set password" / "register" / "link account" שמקבל email: +1. אם חשבון קיים תחת ה-email דרך OAuth (או כל ספק זהות אחר), **דחה** את קביעת הסיסמה אלא אם הבקשה מאומתת כאותו משתמש או כוללת קישור one-time מאומת שנשלח ל-email שמקושר ל-OAuth. +2. ידיעת ה-email לבדה אף פעם לא מספיקה להעברת privilege. + +### ראה גם +- `bugbot-rules/auth-before-irreversible-action.md` +- `bugbot-rules/privilege-escalation-unverified.md` + +--- + +## K2. עקיפת rate limiter דרך זיוף X-Forwarded-For + +**מקור:** 8-Projects C2 (Shipment-bot `11e7379`, routine `06ca796` — שני פרויקטים) +**חומרה:** CRITICAL — בקרת אבטחה נעקפת + +### איך זה נראה +ה-rate limiter קורא `X-Forwarded-For` (או `X-Real-IP`) ישירות מהבקשה בלי לוודא שה-peer המיידי הוא proxy אמין. התוקף מזייף את ה-header → כל בקשה נראית מגיעה מ-IP שונה → אין הגבלה. + +### כלל לזיהוי +בכל מקום שהקוד משתמש ב-`request.headers["x-forwarded-for"]` או שווה ערך להחלטות security / rate-limit / abuse: +1. ה-framework חייב להיות מוגדר עם רשימה מפורשת של proxies אמינים (Starlette `ProxyHeadersMiddleware` עם `trusted_hosts`; Express `app.set('trust proxy', )`; Flask `ProxyFix(... trusted_hops=N)`). +2. **לעולם** אל תסמוך על כל ה-chain של `X-Forwarded-For`; סמוך רק על הסגמנטים מימין השווים למספר ה-hops שמוגדר. +3. אם אין proxy מקדים, השתמש ב-`request.client.host` / `req.socket.remoteAddress` בלבד. + +### ראה גם +- `BY-STACK/webhooks.md` — סעיף על זהות הקורא +- `bugbot-rules/rate-limit-xff-spoofing.md` + +--- + +## K3. הסלמת הרשאות — auto-admin לפי email לא מאומת + +**מקור:** 8-Projects C10 (Markdown-Academy — commit `4623bdb`) +**חומרה:** CRITICAL — תפקיד admin ניתן לתוקף + +### איך זה נראה +ב-registration, הקוד בודק אם ה-email שסופק שווה ל-email של "owner / admin" שמוגדר. אם כן, המשתמש החדש מקבל תפקיד admin — **לפני** שה-email אומת. תוקף שיודע את ה-email של ה-owner יוצר חשבון, לעולם לא מאשר email, והוא admin. + +### כלל לזיהוי +כל נתיב קוד שמעניק תפקיד מוגבר (admin / staff / owner / super_user) חייב: +1. להיות נגיש **רק אחרי** email verification (verification token תקף, `email_verified_at` מסומן). +2. או להיות gated על ידי פעולה out-of-band של admin קיים (קישור invite + token). +3. לעולם לא לסמוך על `request.body.email == OWNER_EMAIL` בזמן registration. + +### ראה גם +- `bugbot-rules/privilege-escalation-unverified.md` + +--- + +## K4. XSS דרך `innerHTML` עם display name של משתמש + +**מקור:** 8-Projects C9 (Facebook-Leads-New — commit `6a6ec51`) +**חומרה:** CRITICAL — DOM XSS בפאנל admin + +### איך זה נראה +`renderBlockedUsers` (או כל פאנל admin/פנימי) השתמש ב-`element.innerHTML = '
' + user.name + '
'` עם שם שמקורו במערכת חיצונית (Facebook display name). משתמש עם שם `` מריץ script בקונטקסט של ה-admin. + +### כלל לזיהוי +1. `.innerHTML = ` / `dangerouslySetInnerHTML` / `v-html` עם מחרוזת שכוללת ערך כלשהו ממקור חיצוני (תגובת API, שורת DB, URL param, `localStorage`) → דווח. +2. ברירת מחדל: `textContent` / text nodes ב-JSX של React. +3. אם HTML באמת נדרש, הערך חייב לעבור דרך `DOMPurify` (או שווה ערך) באותה שורה של ההשמה. +4. "Admin only" אינה הגנה — admins הם בדיוק היעדים של XSS. + +### ראה גם +- `BY-STACK/react-frontend.md` — סעיף DOM +- `bugbot-rules/xss-innerhtml.md` + +--- + +## K5. פאנל admin חשוף לרשת בלי auth + +**מקור:** 8-Projects C11 (Amazon-bot — commits `85776b5`, `c27c769`) +**חומרה:** CRITICAL — RCE / שליטת admin לכל מי שברשת + +### איך זה נראה +שרת Flask / FastAPI / Express שמאזין ב-`0.0.0.0` ("listen on all interfaces") בלי token gate, בלי בדיקת `Authorization` header, בלי IP allowlist. כל מי שמגיע ל-port יכול להשתמש בפאנל. + +### כלל לזיהוי +לכל עליית שרת HTTP: +1. אם נקשר ל-`0.0.0.0` / `::` / "all interfaces" → דרוש או (a) middleware של אימות שדוחה בקשות לא מאומתות *לפני* שכל route רץ, או (b) firewall / ingress שחוסם גישה חיצונית. +2. אם אף אחד לא מוגדר → קשר ל-`127.0.0.1` / `localhost` בלבד. +3. ברירת מחדל: localhost ב-dev; דרוש env var מפורש לקישור פומבי. + +### ראה גם +- `bugbot-rules/network-exposed-without-auth.md` + +--- + +## K6. password hash / secret נדלף ב-response או error message + +**מקור:** 8-Projects C6 (routine `06ca796`), C24 (Shipment-bot `59a5e3c`) +**חומרה:** CRITICAL — חשיפת חומר סודי + +### איך זה נראה +- middleware של `ctx.user` חשף את שורת ה-ORM המלאה כולל שדה `passwordHash`. כל endpoint שעושה serialization למשתמש (profile, comments author, mentions) דלף hashes. +- `InsufficientCreditError.to_dict()` כלל את `self.message = "Insufficient credit for courier {id}"` — UUID פנימי של courier נחשף ב-response של ה-API. + +### כלל לזיהוי +1. Serialization של משתמש / actor חייב לעבור דרך DTO / Pydantic response model מפורש שמפרט רק את השדות המותרים. ORM row → JSON אסור בגבולות API. +2. שמות שדות אסורים בכל מקום ב-response של API: `password`, `passwordHash`, `password_hash`, `salt`, `refresh_token`, `access_token`, `api_key`, `secret`, `private_key`. +3. מחלקות exception שעלולות להיזרק בגבולות API לא יכולות לכלול IDs פנימיים / hostnames / סיבות heuristic ב-message הפומבי שלהן. דפוס: `class XError(AppException): public_message: str # safe; detail: dict # server-only`. + +### ראה גם +- `bugbot-rules/secret-in-error-response.md` +- K13 — הקצה השני: סוד שדולף לערוצים פנימיים (לוגים/Sentry) דרך מחרוזת נגזרת + +--- + +## K7. PII בלוגים וב-API responses + +**מקור:** EmailFlow P5 (מסמך בעברית) + 8-Projects C6, C24, C57 — **RECURRING לפי תדירות, CRITICAL לפי חומרה** +**חומרה:** CRITICAL — פרטיות / GDPR / compliance + +### איך זה נראה +- `logger.info("sending email to %s", user.email)` — email הוא PII; שורד ב-log aggregator לנצח. +- `HTTPException(detail=str(exc))` — מחרוזת exception פנימית עם stack trace דולפת ללקוח. +- `summary: "heuristic: esp:mailchimp.com"` ב-response של API — חושף לוגיקה פנימית של classification. +- הודעות שגיאה באנגלית עם stack traces שמוצגות למשתמשי קצה (במקום הודעה מתורגמת גנרית). + +### כלל לזיהוי +בכל קריאת `logger.*()` וב-`HTTPException` `detail` / body של response: + +**אסור בלוגים (או ב-response של API):** +- `email`, `phone`, `from_email`, `to_email`, שדות כתובת, שדות שם. +- תוכן body של email / chat / messages. +- OAuth tokens גולמיים, `refresh_token`, `access_token`, API keys. + +**אסור ב-response של API בלבד (לוגים בסדר אם יש בקרת גישה ללוגים):** +- סיבות heuristic להחלטה (`'esp:mailchimp.com'`, `'spam_score=0.8'`). +- UUIDs פנימיים של DB של tenants / users אחרים. +- Stack traces, שמות מחלקות exception, הודעות framework. +- שברי SQL. + +**החלפות:** +- PII → רק *domain* של email, או hash לא הפיך, או user_id משלך. +- לוגיקה פנימית → הודעת שגיאה מתורגמת גנרית. +- IDs פנימיים → רק IDs של המשאבים של המשתמש *הנוכחי*. + +### ראה גם +- `RECURRING-PATTERNS.md` מציין שזה הדפוס היחיד מ-RECURRING שגם קודם ל-CRITICAL. +- `bugbot-rules/pii-in-logs.md` + +--- + +## K8. LIKE wildcard injection ב-prefix של משתמש + +**מקור:** 8-Projects C12 (Facebook-Leads-New — commit `2f45eca`) +**חומרה:** CRITICAL — מניפולציה של query / חשיפת נתונים + +### איך זה נראה +`get_config_by_prefix(prefix)` הריץ `WHERE key LIKE :prefix || '%'`. SQL `LIKE` מתייחס ל-`_` כ-"כל תו יחיד" ול-`%` כ-"כל רצף". prefix מהמשתמש כמו `test_key` תפס גם `test_key_foo` וגם `testXkey_foo` — ו-prefix של `%` היה תופס הכל. + +### כלל לזיהוי +לכל סעיף `LIKE` שנבנה מקלט משתמש: +1. ברח מ-`_` ומ-`%` (ומתו ה-escape עצמו) בקלט: `prefix.replace('\\','\\\\').replace('%','\\%').replace('_','\\_')` ושימוש ב-`LIKE :p ESCAPE '\\'`. +2. או שכתב להתאמה מדויקת (`=`) אם חיפוש prefix לא באמת נדרש. +3. SQLAlchemy: השתמש ב-`column.startswith(value, autoescape=True)` במקום `LIKE` ידני. + +### ראה גם +- `BY-STACK/postgres.md` +- `bugbot-rules/like-wildcard-injection.md` + +--- + +## K9. credential auth נשלח לפני storage (OTP / link / token) + +**מקור:** 8-Projects C4 (Shipment-bot — commits `552f0f7`, `155aa81`) +**חומרה:** HIGH — חוסר עקביות ב-lifecycle של auth, אפשרות לעקיפה דרך נתיב אימות חלופי + +### איך זה נראה +סדר הפעולות: +1. ייצור OTP. +2. שליחה דרך SMS / email (בלתי הפיך). +3. שמירה ב-Redis / DB. + +אם שלב 3 נכשל (Redis למטה, שגיאה זמנית), למשתמש יש קוד אמיתי ביד אבל אף verifier לא יוכל להתאים אותו. גרוע מכך, אם יש נתיב fallback "דלג על OTP" שמופעל על "Redis miss", התוקף שיכול לגרום ל-Redis להיות flaky מקבל עקיפה. + +### כלל לזיהוי +כל flow של "ייצור + שליחה של credential" חייב: +1. להתמיד (Redis SET / DB INSERT עם TTL) **לפני** השליחה החיצונית. +2. אם ההתמדה נכשלת, לא לשלוח. +3. אין נתיב "fallback" שממשיך כשאחסון verification לא בר השגה — fail closed. + +הכללה: כל triplet של "reserve / dispatch / store" שהפעולה החיצונית בו בלתי הפיכה — ראה CORE U1 (reserve-then-fill). + +### ראה גם +- `CORE-PATTERNS.md` U1 +- `BY-STACK/webhooks.md` +- `bugbot-rules/auth-before-irreversible-action.md` + +--- + +## K10. 500 גנרי מוצג כ-"invalid credentials" + +**מקור:** 8-Projects C57 (Web — commit `5196657`) +**חומרה:** HIGH — security UX + סיוע לאיתור משתמשים + מסתיר outages אמיתיים + +### איך זה נראה +Handler של login: +```js +catch (err) { + if (err.status === 500) showError("Invalid username or password"); + if (err.status === 401) showError("Invalid username or password"); +} +``` +כל כשל DB, רעש רשת, או באג ב-backend מופיע כ-"wrong password". שלוש תוצאות: +1. Outage אמיתי בלתי נראה ל-ops (משתמשים אומרים "הסיסמה שלי לא עובדת"). +2. עוזר לתוקפים שמחפשים enumeration של חשבונות (כל email נראה "שגוי"). +3. משתמשים נועלים את עצמם מחוץ לחשבונות אמיתיים בניסיון "לאפס" סיסמאות שכן עובדות. + +### כלל לזיהוי +טיפול בשגיאות login / auth חייב להסתעף: +- `401 / 403` → "Invalid credentials" (גנרי, לא דולף אם ה-email קיים). +- `5xx` → "Service temporarily unavailable. Please try again in a moment." + התראה למוניטורינג. +- `429` → "Too many attempts. Try again in N minutes." + +לעולם אל תאחד `5xx` ל-"invalid credentials". + +### ראה גם +- `bugbot-rules/auth-before-irreversible-action.md` (סעיף auth UX) + +--- + +## K11. כשל שמדווח בערך החזרה נבלע — הצלחה מדומה למשתמש + +**מקור:** CodeBot P1 (PR #3232, PR #3172, ומופע cache invalidation) — 3 מופעים באותו ריפו +**חומרה:** HIGH — אובדן נתונים שקט: המשתמש מקבל אישור על פעולה שלא קרתה + +### איך זה נראה +פונקציה מסמנת כשל בערך החזרה — `None`, `False`, `0`, `[]`, rowcount — ולא +בזריקת חריגה. הקורא עוטף אותה ב-`try/except`, מניח ש"לא נזרקה חריגה" פירושו +הצלחה, ומדווח ✅: + +```python +try: + saved = await asyncio.to_thread(backup_manager.save_backup_bytes, raw, metadata) + saved = True # save_backup_bytes מחזיר None בכשל — לא זורק +except Exception: + await notify_failure() # לא ירוץ לעולם +# המשתמש רואה "✅ נשמר גיבוי" על גיבוי שלא נשמר +``` + +הנזק הקשה הוא לא הכשל עצמו אלא **האישור השקרי**: משתמש שסומך על גיבוי +שאינו קיים, קאש שמוגש כמעודכן אחרי invalidation שמחק 0 מפתחות, "נשלח" +שלא נשלח. ה-try/except המיותר מחמיר — הוא משדר לקורא הבא "הכשל מטופל". + +### כלל לזיהוי +לכל קריאה שעטופה ב-try/except או שאחריה דיווח הצלחה: +1. בדוק את מסלול הכשל של הפונקציה הנקראת — זורקת או מחזירה falsy? (הקובע + הוא החוזה של הפונקציה, לא הערך: `0` מ-invalidation שאמור למחוק הוא + כשל, `0` ממחיקה אופורטוניסטית הוא תקין.) +2. אם מחזירה falsy בכשל: חובה `if not result:` אחרי הקריאה, לפני כל דיווח הצלחה. +3. דגל אדום: השמת דגל הצלחה קבוע (`saved = True`) מיד אחרי הקריאה. +4. בכתיבת פונקציה חדשה: בחר ערוץ כשל אחד — עדיף זריקה — ותעד אותו ב-docstring. + +### False positives +- פונקציות raise-on-error מתועדות — שם try/except הוא הערוץ הנכון. +- fire-and-forget מוצהר (מטריקות, לוגים) שכשל שקט מקובל בו. + +### מצב מומלץ +**strict** בכל נתיב שמסתיים בהודעת הצלחה למשתמש או בכתיבת נתונים. + +### ראה גם +- `bugbot-rules/return-value-failure-unchecked.md` +- `bugbot-rules/sdk-error-completeness.md` §3 — קרוב המשפחה בעולם ה-SDK: כשלים שחוזרים כערכים מ-`gather(return_exceptions=True)` +- `CORE-PATTERNS.md` U1 — בדיקת rowcount אחרי CAS היא מופע של אותו עקרון + +--- + +## K12. שאילתה רב-דיירית בלי tenant scope — דליפה בין לקוחות + +**מקור:** EmailFlow (3 מופעי auth/tenant isolation, HIGH — נספח "מחוץ ל-Top 7"); הקשר חי: ai-business-bot (ContextVar של tenant עם `default=None`) +**חומרה:** CRITICAL — דליפת נתונים בין לקוחות: הלקוח של דנה רואה את הלידים של יוסי + +### איך זה נראה +במערכת רב-דיירית, שאילתה אחת ששכחה `WHERE tenant_id = :current` מספיקה כדי שנתוני לקוח אחד יגיעו ללקוח אחר. הדליפה שקטה לגמרי — אין חריגה, אין לוג, התשובה "תקינה" רק עם נתונים של מישהו אחר. שלושת המסלולים הקלאסיים: + +1. **get-by-id בלי scope.** `SELECT ... WHERE id = :id` — ה-id הגיע מהלקוח, וכל מי שמנחש/מונה ids קורא שורות של דיירים אחרים (IDOR). +2. **נתיב משני שנשכח.** ה-list הראשי מסונן, אבל ה-export / החיפוש / ה-aggregation / ה-webhook handler — לא. הבידוד חזק בדיוק כמו השאילתה הכי חלשה. +3. **Context של tenant עם fallback שקט.** `ContextVar("tenant", default=None)` או `default=DEFAULT_TENANT` — נתיב ששכח לקבוע context לא נכשל, הוא רץ על הדייר הלא נכון. גרוע מדליפה: כתיבה לדייר הלא נכון. + +### כלל לזיהוי +1. חוזה לפי סוג הפעולה, בטבלה עם עמודת `tenant_id`/`account_id`/`org_id`: **SELECT / UPDATE / DELETE** — predicate על ה-tenant המאומת בכל שאילתה, כולל get-by-id, exports, aggregations, חיפוש ומחיקות. **INSERT / UPSERT** — אין WHERE שיציל אותך: ה-`tenant_id` הנכתב נגזר מה-context המאומת של הבקשה, לעולם לא מערך שהגיע מהלקוח; וב-UPSERT מפתח ה-conflict כולל את ה-tenant, אחרת דייר אחד דורס שורה של אחר. +2. עדיף אכיפה מרכזית על משמעת נקודתית: scoped session / query builder שמזריק את הסינון, RLS ב-Postgres, או repository שמקבל tenant כפרמטר חובה. +3. Context של tenant: **fail-closed** — בלי default; נתיב בלי context זורק, לא נופל בשקט לברירת מחדל. דגל `TENANCY_STRICT` כבוי בפרודקשן = הדפוס הזה בהמתנה. +4. מפתחות cache, session, וקבצים זמניים כוללים את ה-tenant — אחרת ה-cache מגיש נתוני דייר אחד לאחר. +5. בדיקת בידוד היא בדיקת חובה: שני tenants **סינתטיים** על מסד בדיקה זמני, פעולה זהה, ואימות ששום תשובה לא מכילה נתונים של השני. שלב ה-T2 — הרצה עם ההגנה כבויה כדי לוודא שהבדיקה נופלת — רץ **רק** ב-local או ב-CI על המסד הזמני; לעולם לא בפרודקשן, ב-staging, או בכל סביבה עם נתונים משותפים. + +### False positives +- טבלאות גלובליות באמת (קונפיג מערכת, קטלוג ציבורי) — מסומנות ככאלה במפורש. +- קוד admin שמוצהר ומאובטח ככזה (עם audit log), שסורק את כל הדיירים בכוונה. + +### מצב מומלץ +**strict** על כל טבלה עם עמודת tenant. אין warning-period לדפוס הזה — הדליפה הראשונה היא כבר אירוע אבטחה. + +### ראה גם +- `bugbot-rules/tenant-row-scoping.md` +- `CORE-PATTERNS.md` U5 — עדכון חלקי (הכתיבה מסוננת, הלוג לא) +- `bugbot-rules/privilege-escalation-unverified.md` — הקצה השני: מי בכלל מקבל להיות באיזה tenant + +--- + +## K13. סוד רוכב על מחרוזת נגזרת — דליפה ללוגים ול-Sentry דרך הודעות שגיאה + +**מקור:** CodeBot Pattern 5 (PR #3234, אוגוסט 2026) +**חומרה:** CRITICAL — חשיפת credential בערוצי תצפית, בלי שאף שורה "רושמת סוד" במפורש + +### איך זה נראה +- URL של Telegram Bot API (`.../bot/method`) הוצמד להודעת חריגה + (`msg += f" url={url}"`) ולשדה `e.url`. כל כשל רשת נשא את הטוקן המלא + ללוגים, ל-traceback ולאירועי Sentry. +- הניקוי שהיה קיים נכשל-פתוח: סריקה לפי שמות שדות שמפספסת את גוף + החריגה, `except: pass` שמחזיר את הערך הגולמי, וניקוי עמוק שמחזיר את + המקור בטיפוס לא מוכר. + +### כלל לזיהוי +1. כל בניית מחרוזת שמרכיבה סוד — f-string, `.format()`, `%`, שרשור, + URL builder — לעקוב לאן הערך זורם (source-to-sink). אם הוא מגיע + להודעת חריגה, לשדה על אובייקט חריגה, לכל שיטת לוג, או ל-API של + ניטור (capture_*, breadcrumbs) — חובה לנקות **בנקודת ההרכבה או + בהשמה**, לא אצל הצרכנים. +2. מנגנון ניקוי (redaction) חייב להיכשל-סגור: כשל המרה מחזיר placeholder; + כשל ב-before_send מפיל את האירוע; טיפוס לא מוכר לא חוזר גולמי אם + הייצוג הטקסטואלי שלו נושא סוד. +3. סריקה לפי שמות שדות (`if "token" in key`) אינה ניקוי — גוף חריגה, + breadcrumbs ו-repr של אובייקטים לא עוברים דרכה. סורקים את המבנה כולו. + +### מצב מומלץ +נקודת ניקוי מרכזית אחת; החריגה מנקה את שדותיה בהשמה; רשימת דפוסים אחת +ללוגים (הודעה + traceback); before_send נכשל-סגור עם אזהרה. + +### ראה גם +- `bugbot-rules/secret-in-derived-text.md` +- K6 — הקצה השני: סוד שדולף **ללקוח** ב-response; כאן הדליפה פנימית +- `bugbot-rules/side-effect-riding-on-log-line.md` — התאום ההפוך: שם סוד רוכב על טקסט נגזר ודולף החוצה, כאן פעולה רוכבת על טקסט לוג ונעלמת פנימה כשמנקים אותו. תיקון של K13 הוא הטריגר הנפוץ ביותר לדפוס ההוא. +- `docs/source-projects/codebot-patterns.md` Pattern 5 + + +## K14. סוד בשורת שאילתה — SDK הניטור מתעד אותו בעצמו, ורשימת הניקוי לא מכירה אותו + +**מקור:** CodeBot Pattern 8 (PR #3270, אוגוסט 2026) +**חומרה:** CRITICAL — credential של ספק חיצוני נכתב לערוץ תצפית בכל בקשה מוצלחת + +### איך זה נראה +- סוד מועבר כפרמטר URL — `params={"key": api_key}`, `?token=...` — צורה + שמופיעה בתיעוד של ספקים רבים. אף שורה בקוד לא רושמת אותו לשום מקום. +- אינטגרציית ה-HTTP של ה-SDK לניטור קוראת את הכתובת בעצמה ורושמת אותה. + ב-sentry-sdk **2.42.1 — הגרסה שנבדקה** — זה + `parse_url(str(request.url), sanitize=False)` ואז + `span.set_data(SPANDATA.HTTP_QUERY, parsed_url.query)` — לא מותנה + ב-`send_default_pii` ולא בשום דגל אחר. אותו צמד נמצא גם באינטגרציות + aiohttp ו-stdlib (שמכסה את requests/urllib) של אותה גרסה. +- **האינטגרציה אינה מופיעה בקונפיגורציה.** ב-2.42.1 היא ב- + `_AUTO_ENABLING_INTEGRATIONS` ונדלקת לבד כשהספרייה מותקנת; חיפוש + ברשימת ה-`integrations=[...]` שב-`init` לא ימצא אותה. **גרסאות אחרות + מחייבות אימות נפרד לפני שמסתמכים על ההתנהגות הזו.** +- הרישום קורה בכל בקשה **מוצלחת**, לא רק בכשל — בניגוד לדליפה דרך + הודעת חריגה, שדורשת תקלה. +- החצי השני: רשימת דפוסי הניקוי מזהה **צורות של סודות מוכרים** + (`ghp_`, `Bearer`, טוקן טלגרם). כל ספק חדש הוא דליפה חדשה עד שמישהו + נזכר להוסיף רג'קס. + +### כלל לזיהוי +1. סוד שנכנס לכתובת בקשה הוא ממצא כשמותקן SDK ניטור עם אינטגרציית + HTTP — גם אם הקוד שלך לא נוגע ב-URL אחרי הבנייה. לבדוק + `_AUTO_ENABLING_INTEGRATIONS`, לא רק את מה שנרשם ידנית. **דליפה + מאושרת** דורשת גם מסלול שליחה שלם — init עם DSN, transaction שנדגם, + והאירוע שורד את הניקוי; בלי אימות המסלול, לדווח כחשד/סיכון רדום. +2. רשימת דפוסי ניקוי שכולה דפוסי-צורה היא **רשימה שחורה**. חייב להיות + בה לפחות כלל אחד חסין-ספק — ניקוי לפי **שם** הפרמטר ולא לפי צורת + הערך. +3. כלל לפי שם חייב להתייחס לשם כמקטע: `auth_token`, `oauth_token`, + `id_token`, `private_key`, `x-api-key` הם שמות נפוצים, והתאמה מדויקת + מהמפריד מפספסת את כולם. **מקטע פירושו גבול משני צדדיו** — ושניהם + נדרשים: + - **לפני השם**: תחילת מחרוזת, `?`, `&`, או מפריד בתוך השם. בלי + הגבול הזה `?monkey=` נתפס, כי `monkey=` מכיל `key=` כתת-מחרוזת. + - **אחרי השם**: `=` מיד אחרי המילה הרגישה. בלי זה `?key_id=` ו- + `?token_type=` נתפסים, למרות שהם מזהים ולא סודות. + התחילית עצמה חייבת להסתיים באחד משלושה גבולות: מפריד (`_`, `-`, `.`), + גבול camelCase (`[a-z0-9]` ← `[A-Z]`), או מעבר ספרה ← אות + (`[0-9]` ← `[A-Za-z]`). בלי השני, `apiKey` ו-`clientSecret` מפוספסים; + בלי הספרה בצד השמאלי שלו, `v2Token` ו-`sha256Token` מפוספסים; ובלי + השלישי, `v2token` בכתיב קטן מפוספס — למרות שהוא נושא בדיוק את אותו + credential. **רצף ספרות הוא מפריד מקטע**, ולכן `v2` + `token` היא + קריאה טבעית ואילו `mon` + `key` אינה — וזה מה שמפריד את הגבול הזה + מגבול אות ← אות שהיה תופס את `?monkey=`. וגבול ה-camelCase חייב להיות רגיש-רישיות: בדפוס תחת + `(?i)`, `[A-Z]` תופס גם אותיות קטנות והגבול מתדרדר ל"בין כל שתי + אותיות", מה שמחזיר את `?monkey=`. +4. הכלל חייב לתפוס גם **מחרוזת שאילתה עירומה** (`key=...` בלי `?` + מוביל) — זו הצורה שבה שדות ניטור שומרים אותה. + +### מצב מומלץ +הסוד עובר ל-header ייעודי (`x-goog-api-key` אצל Google), ויושב על +**הלקוח** ולא באתר הקריאה כדי שאתר קריאה חדש יקבל אותו בלי לדעת עליו — +**ובלבד שכל היעדים של אותו לקוח הם של אותו ספק.** כותרת ברירת-מחדל +נשלחת בכל בקשה דרכו, ולקוח משותף שפונה גם ליעדים אחרים ישלח את +ה-credential למי שלא צריך אותו. לקוח ייעודי לספק, או בדיקת host מול +allowlist לפני הוספת הכותרת. + +רשימת דפוסי הניקוי משותפת בין כל הרשתות (Sentry + לוגים) ומכילה כלל +לפי-שם. אימות מול מבנה האירוע האמיתי, לא רק מול דוגמאות שכתבת. + +### ראה גם +- `bugbot-rules/secret-in-url-query.md` +- K13 — הצורה הבסיסית: הקוד שלך מרכיב ושולח ל-sink. כאן אין sink בקוד + שלך; ה-SDK קורא וכותב. "המצב המומלץ" של K13 היה מיושם במלואו ובכל + זאת נכשל, כי הוא אינו דורש שהרשימה תהיה חסינת-ספק. +- `bugbot-rules/secret-in-derived-text.md` — סעיף ה-false-positives שם + תוקן בעקבות הדפוס הזה. +- `docs/source-projects/codebot-patterns.md` Pattern 8 + +--- + +## K15. שומר שמתפרסם לפני הערך שהוא שומר עליו — השבתה שקטה עד ריסטארט + +**מקור:** CodeBot Pattern 11 (PR #3357, ספטמבר 2026) +**חומרה:** HIGH — תקלת רשת חולפת אחת משביתה שירות שלם בשקט, עד ריסטארט + +### איך זה נראה + +אתחול עצל של משאב משותף מחזיק **שני** משתנים — שומר וערך — והמסלול +המהיר בודק את השומר בזמן שהוא מחזיר את הערך: + +```python +def get_db(): + global client, db + if client is None: # ← השומר + with lock: + if client is None: + client = MongoClient(url) # פורסם ראשון + client.server_info() # ← סיבוב רשת שלם + db = client[name] # הערך, אחרון + return db # ← מה שבאמת מוחזר +``` + +שני מסלולי כשל. **בשניהם התסמין הנמשך הוא `None` ולא חריגה** — אבל לא באותו רגע: + +1. **מרוץ.** `server_info()` הוא סיבוב רשת. קורא מקביל שנכנס באמצעו רואה + שומר מאותחל וערך ריק. +2. **הרעלה קבועה — החמורה.** כאן ההבחנה חשובה: הקריאה **הראשונה + זורקת** כמצופה, וה-`except` עושה את עבודתו. אבל היא כבר הספיקה + להשאיר את `client` מוצב על לקוח שבור — ולכן **כל קריאה עתידית** מדלגת + על האתחול ומחזירה `None` לתמיד. הכשל הגלוי קורה פעם אחת; מה שנמשך + הוא השקט אחריו. אינה דורשת מקביליות בכלל. + +### למה זה HIGH ולא סתם באג מקביליות + +הכשל מגיע כ-`None`, ורוב הקוראים מסתעפים על "המשאב לא זמין" ומדרדרים +בחן. **המערכת מדווחת על עצמה כתקינה-חלקית בזמן שהמשאב בריא לגמרי.** אין +חריגה, אין התראה, והתסמין שמגיע למשתמש ("החיפוש לא מוצא") מצביע על +המקום הלא נכון. ב-CodeBot: 211 קריאות ל-`get_db()` בקוד הייצור, 96 מהן +בלי `try` צמוד. + +### כלל לזיהוי + +1. אתחול עצל שבו המשתנה הנבדק במסלול המהיר **אינו** המשתנה המוחזר. +2. פרסום לפני אימות: הצבה גלובלית ואחריה `ping`/`server_info`/`auth`. +3. כשל שמשאיר את השומר מוצב — הבדיקה: *אחרי החריגה, האם קריאה חוזרת + תנסה שוב?* +4. `double-checked locking` שבו הבדיקה החיצונית קוראת משתנה שאינו + האחרון שנכתב בתוך הנעילה. + +### המצב המומלץ + +לבדוק על **הערך המוחזר**, ולבנות את המשאב במשתנים מקומיים שמתפרסמים רק +כשהוא מוכן — הערך קודם, השומר אחריו. ב-CodeBot חמישה מתוך שישה אתרי +אתחול עצל כבר עשו זאת נכון (`if _collection is not None`); אחד סטה, והוא +זה שנקרא מ-211 מקומות. + +### אזהרה: התיקון לבדו הוא רגרסיה + +ההרעלה שבסעיף 2 שימשה גם כמפסק. הסרתה לבדה גררה את הקצה הנגדי — כל +קריאה מנסה מחדש ומשלמת timeout מלא. נמדד, 20 קריאות מול מארח שאינו +נפתר: **5.0 שניות** לפני, **111.0** אחרי התיקון הנאיבי, **5.1** עם מפסק +בעל זמן פתיחה **סופי**. ב-CI זה הרג עובד xdist שלם וארבע ריצות רצופות +נתקעו עד תקרת 30 הדקות. + +**הכלל שרחב מהדפוס: התנהגות מנוונת שקיימת מזמן היא לעיתים תשתית שמישהו +נשען עליה בלי שהצהיר. לפני שמסירים אותה — מודדים מי.** + +### ראה גם +- `bugbot-rules/lazy-init-guard-publish-order.md` +- `bugbot-rules/silent-fallback-to-worse-path.md` — הכשל כאן הוא מופע + שלו: המסלול החלופי ("המשך בלי מסד") עובד, רק גרוע, ואיש לא מדווח. +- `CORE-PATTERNS.md` U1 — משפחת המרוצים. שם הבעיה היא check-then-act על + מצב משותף; כאן היא **סדר הפרסום** של שני משתנים, וסעיף 2 אינו מרוץ + כלל. +- `docs/source-projects/codebot-patterns.md` Pattern 11 + +--- + +## K16. גבול נתיב שנבדק כקידומת מחרוזת — מחיקה מחוץ ל-allowlist + +**מקור:** CodeBot P13 (PR #3249, אוגוסט 2026) +**חומרה:** HIGH — `rmtree` על תיקייה שמחוץ לרשימה המותרת. בלתי הפיך, ונראה בקוד כמו בדיקת בטיחות תקינה + +### איך זה נראה + +השאלה היא "האם הנתיב הזה נמצא **בתוך** התיקייה המותרת". התשובה נכתבת כהשוואת **תווים**: + +```python +def safe_rmtree(p: Path, allow_under: Path): + if not str(p).startswith(str(allow_under)): # ❌ השוואת תווים + raise ValueError("outside allowlist") + shutil.rmtree(p) +``` + +עם `allow_under=/tmp/app-test`, המחרוזת `/tmp/app-test-evil` **עוברת** — היא באמת מתחילה באותם תווים, ואין ביניהן שום קשר היררכי. ‏`shutil.rmtree` מוחק תיקייה שלמה שמחוץ ל-allowlist. שוחזר אמפירית ב-CodeBot לפני התיקון: נתיב-אח נמחק בפועל. + +### התיקון + +```python +p, base = p.resolve(), allow_under.resolve() # ‏..‏ וקישורים סימבוליים קודם +if not (p == base or base in p.parents): # ✅ השוואת נתיב + raise ValueError("outside allowlist") +``` + +‏`parents` היא רשימת תיקיות, לא רצף תווים, ולכן היא אינה יכולה לענות "כן" ל-`-evil`. ה-`resolve()` אינו קישוט: בלעדיו `base/../../etc` עובר את אותה בדיקה. + +**וסייג שהתיקון הזה אינו פותר — TOCTOU:** בין ה-`resolve()` לבין ה-`rmtree` יש חלון. אם גורם לא מהימן יכול להחליף רכיב בנתיב באותו רגע (קישור סימבולי שמוחלף, תיקיית אב שמוזזת), הבדיקה עברה על נתיב אחד והמחיקה תתבצע על אחר. הכלל הזה מניח ש**העץ מהימן ואינו משתנה בזמן הפעולה** — ובסביבה שבה ההנחה אינה מתקיימת נדרש מנגנון אטומי (פתיחת מתאר לתיקייה ועבודה יחסית אליו, `O_NOFOLLOW`, או מרחב שמות ייעודי), לא בדיקת מחרוזת משופרת. + +### המשפחה המלאה — כל גבול היררכי שנבדק כמחרוזת + +| הדומיין | הצורה השבורה | מה עובר אותה | +|---|---|---| +| נתיב קבצים | `str(p).startswith(str(base))` | `/tmp/app-test-evil` | +| URL / origin | `url.startswith("https://api.example.com")` | `https://api.example.com.evil.net` | +| דומיין | `host.endswith("example.com")` | `notexample.com` | +| מפתח קאש / prefix | `key.startswith(f"user:{uid}")` | `user:12` תופס את `user:123` | + +בכל ארבעתם הצורה הנכונה היא להשוות עם המבנה שיודע מה הדומיין — `Path.parents`, גבול מפורש (`base + os.sep`, ‏`"." + domain`, ‏`prefix + ":"`), ו**לכתובות: פירוק ואז השוואה רכיב-רכיב**. + +**ודיוק על URL, כי `hostname` לבדו אינו origin:** ‏origin הוא **scheme + host + port אפקטיבי**. השוואת `urlsplit(u).hostname` לבדה מתאימה ל-allowlist של מארחים ותו לא — היא מקבלת `http://` במקום `https://`, ופורט אחר לגמרי. בכל מקום שבו ההחלטה היא origin — הפניה, בקשה יוצאת, בדיקת `Origin`/`Referer`, ‏`postMessage` — משווים את שלושת הרכיבים, עם הפורט המשתמע כשהוא חסר (443 ל-https, ‏80 ל-http). + +### כלל לזיהוי + +1. `startswith` / `endswith` / `in` על מחרוזת, כשהערך שנבדק הוא נתיב, URL, דומיין או מפתח מרחב-שמות — **ובמיוחד** כשהתוצאה שולטת בפעולה בלתי הפיכה (`rmtree`, ‏`unlink`, ‏`DROP`, הפניה, החלטת הרשאה). +2. בדיקת נתיב בלי `resolve()` / `realpath()` על **שני** הצדדים לפני ההשוואה. +3. השוואת קידומת בלי מפריד מפורש בסופה. + +### False positives + +- קידומת שהיא באמת קידומת טקסטואלית: שם קובץ שמתחיל ב-`tmp_`, בדיקת סכמה `url.startswith("https://")`, ניתוב לפי prefix בשכבת תצוגה. +- ‏`startswith` על ערך פנימי קבוע שלא מגיע מקלט ולא נגזר ממנו. + +### מצב מומלץ + +**strict** כשהתוצאה שולטת במחיקה, בכתיבה או בהחלטת הרשאה. **warning** אחרת. + +### ראה גם +- `bugbot-rules/path-prefix-not-boundary.md` +- `CRITICAL-PATTERNS.md` K8 — `LIKE` wildcard injection: גם שם הטעות היא טיפול בערך מובנה כמחרוזת שטוחה +- `docs/source-projects/codebot-history-scan-patterns.md` P13 diff --git a/tests/fixtures/md_corpus/INTEGRATION.md b/tests/fixtures/md_corpus/INTEGRATION.md new file mode 100644 index 000000000..b66f4e5d2 --- /dev/null +++ b/tests/fixtures/md_corpus/INTEGRATION.md @@ -0,0 +1,270 @@ +# INTEGRATION — איך הידע מהריפו הזה מגיע לסשנים בזמן אמת + +הספרייה הזו שווה בדיוק כמה שהיא נקראת **בזמן המימוש**. המסמך הזה מגדיר +שלושה מנגנונים שמחברים אותה לעבודה השוטפת, ותהליך אחד שמונע ממנה להתיישן. + +**עקרון החלוקה:** `CLAUDE.md` של פרויקט מכיל *טריגרים* — מתי לקרוא ומה. +הריפו הזה מכיל את *הידע* — למה, איך מזהים, ומה false positive. לא מעתיקים +את הידע לפרויקטים (הוא מתיישן שם); מעתיקים רק את ההפניה. + +--- + +## 1. בלוק הטריגרים ל-CLAUDE.md (לכל פרויקט פעיל) + +מדביקים ב-`CLAUDE.md` של הפרויקט את התבנית שלמטה, ואת שורות הטבלה שבתוכה +ממלאים מ-**§2** לפי ה-stack של הפרויקט. שני החלקים יחד הם הבלוק — התבנית +לבדה חסרה את הטריגרים, והשורות לבדן חסרות את הוראת הקריאה. + +דרכי קריאה, לפי מה שזמין בסשן: mirror של הריפו הזה ב-CodeKeeper +(`codekeeper_search_repo` / `codekeeper_get_repo_file`), או ישירות +מ-GitHub (`amirbiron/amir-bug-patterns` הוא ריפו ציבורי). + +### תבנית הבלוק + +```markdown +## דפוסי באגים — amir-bug-patterns + +לפני שאתה נוגע באחד מהנושאים בטבלה — קרא את הקובץ המתאים ב-`amirbiron/amir-bug-patterns` (דרך MCP של CodeKeeper אם יש mirror, אחרת מ-GitHub). +אלה דפוסים שכבר עלו לי בפרודקשן — לא תיאוריה. + +| כשאתה נוגע ב... | קרא | +|---|---| +| <נושא> | <קובץ> | + + +תמיד, בכל פרויקט: +- לפני עטיפת קריאה ב-try/except → `CRITICAL-PATTERNS.md` K11 (כשל בערך החזרה) +- כותב `except` שבולע — `pass`, ערך falsy, או המשך כאילו הצליח → לוג, או הערה שאומרת למה הכשל הזה אינו מעניין. בלעדיהם זה ממצא, גם כשאין ✅ למשתמש במסלול. ‏(`raise` מחדש אינו בליעה) (K11, הצד ההפוך) +- לפני כתיבת טסט חדש → `claude-md-snippets/testing.md` +- אחרי שטסט נופל על חריגה → `bugbot-rules/widened-exception-scope.md` (אל תרחיב except) +- לפני העברת סוד כפרמטר URL (`params={"key": ...}`) → `CRITICAL-PATTERNS.md` K14. ה-SDK של הניטור רושם את השאילתה בעצמו, בכל בקשה מוצלחת. + +### סגירת הלולאה (חובה, לא רשות) +1. **ריוויוור (cubic/qodo/CodeRabbit/claude) תפס דפוס אמיתי** שאינו ב-amir-bug-patterns → פתח שם PR שמוסיף אותו (מסמך מקור + הצלבה לפי ה-README), **וגם** הוסף שורת טריגר לטבלה כאן. +2. **אתה בעצמך זיהית דפוס חוזר אצלך** (תיקנת פעמיים את אותו סוג טעות) → אותו תהליך בדיוק. +3. דפוס בלי שורת טריגר = דפוס שלא ייקרא. שני הצעדים הם צעד אחד. +4. **טריגר מזהה מה אתה מקליד, לא באיזה מצב אתה נמצא.** ‏`LOCK_FAIL_OPEN` ו-`0.0.0.0` רואים על המסך; "מסלול שרץ גם ברקע" ו"אחד מארבעת הנתיבים האלה" הם דברים שצריך **לדעת**. שורה שדורשת את הידע הזה כתנאי כניסה דורשת בדיוק את התשובה שהיא אמורה לתת, ולכן לא תידלק — התנאי שייך למסמך, לא לשורה. ומאותה סיבה: לתאר **מחלקה** ולא למנות מופעים, כי רשימה מתיישנת ברגע שנוסף המופע הבא. + +> [!IMPORTANT] +> **שורה ממופתחת לפי מה שיהיה מול העיניים, לא לפי הקטגוריה של הדפוס.** +> ‏U3 היה מתועד, והייתה לו שורה — `PyGithub / קריאות SDK חיצוני` — והיא לא +> נדלקה על ולידציה של ערך שהגיע מגוף JSON. הסיבה: השורה ממופתחת לפי מה +> שאתה **קורא לו**, והדפוס הוא על מאיפה ה**נתון** הגיע. רוב הדוגמאות של +> U3 אינן קריאות SDK בכלל. מי שכותב את הקוד מסתכל בטבלה, לא מוצא שורה, +> וממשיך. +> +> ולכן גם: **השורה מצביעה על הכלל הכללי ישירות.** הפניה לקובץ סטאק +> שמצליב לכלל היא קפיצה שנייה, ואותה עושים רק אחרי שכבר החלטת שהמקרה +> שלך בצורת אותו סטאק — כלומר בדיוק ההחלטה שהטבלה אמורה לחסוך. +> +> המבחן לפני שכותבים שורה: **האם מי שעומד לכתוב את הקוד יזהה את עצמו +> בניסוח הזה, לפני שהוא יודע מה הבאג?** אם לא — הניסוח לפי הקטגוריה, לא +> לפי המשימה. + +התהליך המלא והמיפוי לשאר הפרויקטים: `INTEGRATION.md` באותו ריפו. +``` + +### למה זה עובד +- `CLAUDE.md` נקרא בכל סשן ממילא — אין תלות בזיכרון של אף אחד. +- הטבלה קצרה (שורות בודדות) — לא מנפחת את הקובץ. +- הידע נשאר במקום אחד ומתעדכן במקום אחד. + +### והסניפטים האוניברסליים (universal / critical / testing)? + +ה-README (סעיף "איך להחיל על **פרויקט חדש**") אומר להדביק את שלושתם — +~70 שורות (עם `hebrew.md` — ~90, המספר שב-README). שני המסמכים לא סותרים, כי הם מדברים על שני מצבים: + +- **פרויקט חדש**: מדביקים את שלושת הסניפטים במלואם (צעד 1 ב-§4 כאן). + אין עדיין CLAUDE.md גדול, וה-70 שורות הן ה-baseline שכל השאר נבנה עליו. +- **פרויקט קיים עם CLAUDE.md מבוסס**: לא מדביקים אף אחד מהם במלואו — זה מנפח קובץ שכבר עמוס ויוצר עותק שמתיישן. מה כן, פר-סניפט: `testing.md` כבר מופנה מבלוק הטריגרים (שורת "לפני כתיבת טסט חדש") — ההפניה מספיקה; `universal.md` ו-`critical.md` נשארים בהפניה דרך הטריגרים של §2, ו**כלל בודד מתוכם שהדפוס שלו כבר עלה בפרויקט נפרס inline בבלוק** — כמו K11 ב-`CLAUDE.md` של CodeBot, שנכתב שם בגוף הטקסט כי הוא תפס את הריפו הזה שלוש פעמים. עלה דפוס נוסף בפרויקט ← מוסיפים גם את הכלל שלו inline, לא את כל הסניפט. + +--- + +## 2. מיפוי פר-פרויקט — מה ממלא את הטבלה שב-§1 + +> ⚠️ **מה שלמטה הוא רק שורות הטבלה, לא הבלוק להדבקה.** +> הטבלאות כאן נראות שלמות (יש להן כותרת ושורת מפריד) — אבל הן רק החלק +> שמשתנה בין פרויקטים. אם תדביק רק אותן, סוכן יראה `CRITICAL-PATTERNS.md` +> ולא יידע שזה ריפו אחר, ולא יקבל את הכללים הקבועים ולא את סגירת הלולאה. + +**איך מרכיבים, בשני צעדים:** + +1. קח את **התבנית המלאה מ-§1** — היא נותנת את המעטפת: הוראת הקריאה + (מאיפה מביאים את הקבצים), שלושת הכללים שחלים תמיד, וסגירת הלולאה. +2. החלף בה את שורת ה-placeholder `| <נושא> | <קובץ> |` בשורות של הפרויקט + שלך מהרשימה כאן — **רק שורות הנתונים**. אל תעתיק את שורת הכותרת + (`| כשאתה נוגע ב... | קרא |`) ואת שורת המפריד (`|---|---|`): הן כבר + קיימות בתבנית, והעתקה שלהן תיצור כותרת כפולה באמצע הטבלה. + +**דוגמה חיה, כבר מודבקת בפועל:** הסעיף *"דפוסי באגים — amir-bug-patterns"* +בסוף [`CLAUDE.md` של CodeBot](https://github.com/amirbiron/CodeBot/blob/main/CLAUDE.md). +שם רואים את התוצאה המורכבת — כולל התאמות מקומיות שהתבנית לא מכתיבה +(שורת "מתי להשתמש" בסגנון הריפו, וכלל K11 שנפרס בגוף הטקסט כי הוא זה +שכבר עלה שם שלוש פעמים). התבנית היא בסיס, לא חוזה נוקשה. + +השורות עצמן, לפי ה-stack של כל פרויקט: + +### CodeBot = CodeKeeper — ריפו אחד: `amirbiron/CodeBot` +(בוט טלגרם + webapp + MongoDB/GridFS + MCP + ChatOps + Sphinx docs. +CodeKeeper הוא שם המוצר, CodeBot הוא שם הריפו — לא שני פרויקטים.) + +| כשאתה נוגע ב... | קרא | +|---|---| +| שמירה/מחיקה שמסתיימת בהודעת ✅ למשתמש | `CRITICAL-PATTERNS.md` K11 | +| קאש / invalidation | `bugbot-rules/return-value-failure-unchecked.md` §4 | +| דגל שמצהיר "מטא-דאטה בלבד" (`npm install --package-lock-only`, `django-admin migrate --fake`), או רשומה שמתעדכנת בנפרד מהעבודה שהיא מתארת | `bugbot-rules/state-record-without-state-change.md` | +| callbacks / handlers מקביליים, מזהים מבוססי-זמן | `CORE-PATTERNS.md` U1 | +| `find_one_and_update` / `upsert` / תפיסת ג'וב | `CORE-PATTERNS.md` U1 + `bugbot-rules/race-toctou.md` | +| PyGithub / קריאות SDK חיצוני | `BY-STACK/external-sdk.md` | +| **ערך שהגיע מחוץ לתהליך** — גוף JSON, payload, כותרת, ENV, פלט של LLM, CSV, או ערך שמשתמש הקליד | `CORE-PATTERNS.md` U3 + `bugbot-rules/external-input-isinstance.md` | +| חיתוך או מדידת אורך של טקסט (`$substrBytes`, `$strLenBytes`, `encode()[a:b]`, תקרת אורך שנשלחת החוצה) | `BY-STACK/hebrew-source.md` H6 + `bugbot-rules/hebrew-source-and-data.md` §6 | +| הרכבת URL/מחרוזת שמכילה סוד, הודעות חריגה, ניקוי לוגים/Sentry | `CRITICAL-PATTERNS.md` K13 + `bugbot-rules/secret-in-derived-text.md` | +| קריאה ל-API עם מפתח/טוקן בפרמטרים, או שינוי ברשימת דפוסי הניקוי | `CRITICAL-PATTERNS.md` K14 + `bugbot-rules/secret-in-url-query.md` | +| מסיר שורת לוג, או עוטף אותה ב-guard שמונע הערכת ארגומנטים (הטריגר הנפוץ: תיקון PII) | `bugbot-rules/side-effect-riding-on-log-line.md` | +| גייטינג אדמין — רשימת היתר, התחזות (`/admin/impersonate/*`), או דגל שמרחיב הרשאה בהיעדר קונפיג (`CHATOPS_ALLOW_ALL_IF_NO_ADMINS`) | `CRITICAL-PATTERNS.md` K3 + `bugbot-rules/privilege-escalation-unverified.md`; לדגל עצמו — K12 סעיף 3 (fail-closed, בלי default שקט) | +| `LOCK_FAIL_OPEN` — מריץ polling בלי מנעול ה-singleton, כלומר שני מריצים במקום אחד | `CORE-PATTERNS.md` U1 | +| העלאת שרת HTTP או שינוי כתובת האזנה (`0.0.0.0`) | `CRITICAL-PATTERNS.md` K5 + `bugbot-rules/network-exposed-without-auth.md` | +| `docs/**/*.rst` | `bugbot-rules/line-number-coupling.md` | +| טסטים עם סטאבים ידניים | `TESTING-PATTERNS.md` + `bugbot-rules/widened-exception-scope.md` | +| אתחול עצל של משאב משותף (חיבור, לקוח, pool, קאש) — או **הסרה** של התנהגות מנוונת שקיימת מזמן | `CRITICAL-PATTERNS.md` K15 + `bugbot-rules/lazy-init-guard-publish-order.md` | +| `getattr(x, "y", None)` או `except` שאחריו **מסלול חלופי בגלל כשל** — לא ערך ברירת מחדל, ולא זיהוי יכולת סטטי | `bugbot-rules/silent-fallback-to-worse-path.md` | +| CSP, כותרות תגובה, או עמוד שנגיש בלי התחברות | `BY-STACK/browser-policy.md` | +| `create_index` — ובמיוחד `partialFilterExpression` או `sparse=True` | `BY-STACK/mongodb.md` דפוס 1 + `bugbot-rules/mongo-index-and-operator-traps.md` — אופרטורים שהפילטר החלקי לא מקבל, ואתחול שבולע את השגיאה | +| צינור `aggregate` — `$project`, `$sort`, `$group` — או `find_one` בתוך לולאה | `RECURRING-PATTERNS.md` R8 + `bugbot-rules/work-disproportionate-to-answer.md` | +| `startswith` / `endswith` על נתיב, URL או דומיין | `CRITICAL-PATTERNS.md` K16 | +| `replace(tzinfo=` · `datetime.now()` **בלי** אזור זמן · כל `date.today()` · הצגת תאריך למשתמש | `RECURRING-PATTERNS.md` R7 | +| **ברמה העליונה של מודול** (מחוץ לכל פונקציה): `Thread(`, `.start()`, `scheduler`, `asyncio.create_task`, לקוח או חיבור שנבנה מ-`os.environ`, או קריאת רשת, מסד או קובץ | `bugbot-rules/import-time-side-effects.md` | +| שינוי שנעשה כדי לספק לינטר: הזזת `import`, ניקוי אזהרת escape, הרחבת `except` | `bugbot-rules/linter-fix-changes-runtime-behavior.md` | +| מחיקה / שיתוף / שינוי שם לפי `_id` שהגיע מהממשק, ‏`created_at`, או פעולה גורפת על `code_snippets` ו-`large_files` | `bugbot-rules/logical-entity-vs-version-document.md` | +| שליחה לספק עם תקרת קלט (טוקנים, אורך שדה), או `value[:LIMIT]` לפני שמירה | `bugbot-rules/silent-truncation-at-sink.md` | +| `clientX` בחיסור, ‏`left`/`right`, גרירה, שינוי גודל, או הצמדה ל-viewport | `BY-STACK/hebrew-source.md` H7 + `bugbot-rules/rtl-geometry-and-clamp.md` | +| `EWMA` / `_ewma`, ‏`anomaly_detected`, ‏`cooldown`, ‏`first_ts`, ‏`adaptive_*_threshold_*` | `RECURRING-PATTERNS.md` R9 + `BY-STACK/observability.md` | +| הרצה ראשונה של ג'וב סנכרון: `baseline`, `last_*_check_time`, `if not last_seen` | `BY-STACK/cron-jobs.md` דפוס 10 — מה ההרצה הראשונה שולחת | +| תגית `` מריץ script בקונטקסט של ה-admin. - -### כלל לזיהוי -1. `.innerHTML = ` / `dangerouslySetInnerHTML` / `v-html` עם מחרוזת שכוללת ערך כלשהו ממקור חיצוני (תגובת API, שורת DB, URL param, `localStorage`) → דווח. -2. ברירת מחדל: `textContent` / text nodes ב-JSX של React. -3. אם HTML באמת נדרש, הערך חייב לעבור דרך `DOMPurify` (או שווה ערך) באותה שורה של ההשמה. -4. "Admin only" אינה הגנה — admins הם בדיוק היעדים של XSS. - -### ראה גם -- `BY-STACK/react-frontend.md` — סעיף DOM -- `bugbot-rules/xss-innerhtml.md` - ---- - -## K5. פאנל admin חשוף לרשת בלי auth - -**מקור:** 8-Projects C11 (Amazon-bot — commits `85776b5`, `c27c769`) -**חומרה:** CRITICAL — RCE / שליטת admin לכל מי שברשת - -### איך זה נראה -שרת Flask / FastAPI / Express שמאזין ב-`0.0.0.0` ("listen on all interfaces") בלי token gate, בלי בדיקת `Authorization` header, בלי IP allowlist. כל מי שמגיע ל-port יכול להשתמש בפאנל. - -### כלל לזיהוי -לכל עליית שרת HTTP: -1. אם נקשר ל-`0.0.0.0` / `::` / "all interfaces" → דרוש או (a) middleware של אימות שדוחה בקשות לא מאומתות *לפני* שכל route רץ, או (b) firewall / ingress שחוסם גישה חיצונית. -2. אם אף אחד לא מוגדר → קשר ל-`127.0.0.1` / `localhost` בלבד. -3. ברירת מחדל: localhost ב-dev; דרוש env var מפורש לקישור פומבי. - -### ראה גם -- `bugbot-rules/network-exposed-without-auth.md` - ---- - -## K6. password hash / secret נדלף ב-response או error message - -**מקור:** 8-Projects C6 (routine `06ca796`), C24 (Shipment-bot `59a5e3c`) -**חומרה:** CRITICAL — חשיפת חומר סודי - -### איך זה נראה -- middleware של `ctx.user` חשף את שורת ה-ORM המלאה כולל שדה `passwordHash`. כל endpoint שעושה serialization למשתמש (profile, comments author, mentions) דלף hashes. -- `InsufficientCreditError.to_dict()` כלל את `self.message = "Insufficient credit for courier {id}"` — UUID פנימי של courier נחשף ב-response של ה-API. - -### כלל לזיהוי -1. Serialization של משתמש / actor חייב לעבור דרך DTO / Pydantic response model מפורש שמפרט רק את השדות המותרים. ORM row → JSON אסור בגבולות API. -2. שמות שדות אסורים בכל מקום ב-response של API: `password`, `passwordHash`, `password_hash`, `salt`, `refresh_token`, `access_token`, `api_key`, `secret`, `private_key`. -3. מחלקות exception שעלולות להיזרק בגבולות API לא יכולות לכלול IDs פנימיים / hostnames / סיבות heuristic ב-message הפומבי שלהן. דפוס: `class XError(AppException): public_message: str # safe; detail: dict # server-only`. - -### ראה גם -- `bugbot-rules/secret-in-error-response.md` -- K13 — הקצה השני: סוד שדולף לערוצים פנימיים (לוגים/Sentry) דרך מחרוזת נגזרת - ---- - -## K7. PII בלוגים וב-API responses - -**מקור:** EmailFlow P5 (מסמך בעברית) + 8-Projects C6, C24, C57 — **RECURRING לפי תדירות, CRITICAL לפי חומרה** -**חומרה:** CRITICAL — פרטיות / GDPR / compliance - -### איך זה נראה -- `logger.info("sending email to %s", user.email)` — email הוא PII; שורד ב-log aggregator לנצח. -- `HTTPException(detail=str(exc))` — מחרוזת exception פנימית עם stack trace דולפת ללקוח. -- `summary: "heuristic: esp:mailchimp.com"` ב-response של API — חושף לוגיקה פנימית של classification. -- הודעות שגיאה באנגלית עם stack traces שמוצגות למשתמשי קצה (במקום הודעה מתורגמת גנרית). - -### כלל לזיהוי -בכל קריאת `logger.*()` וב-`HTTPException` `detail` / body של response: - -**אסור בלוגים (או ב-response של API):** -- `email`, `phone`, `from_email`, `to_email`, שדות כתובת, שדות שם. -- תוכן body של email / chat / messages. -- OAuth tokens גולמיים, `refresh_token`, `access_token`, API keys. - -**אסור ב-response של API בלבד (לוגים בסדר אם יש בקרת גישה ללוגים):** -- סיבות heuristic להחלטה (`'esp:mailchimp.com'`, `'spam_score=0.8'`). -- UUIDs פנימיים של DB של tenants / users אחרים. -- Stack traces, שמות מחלקות exception, הודעות framework. -- שברי SQL. - -**החלפות:** -- PII → רק *domain* של email, או hash לא הפיך, או user_id משלך. -- לוגיקה פנימית → הודעת שגיאה מתורגמת גנרית. -- IDs פנימיים → רק IDs של המשאבים של המשתמש *הנוכחי*. - -### ראה גם -- `RECURRING-PATTERNS.md` מציין שזה הדפוס היחיד מ-RECURRING שגם קודם ל-CRITICAL. -- `bugbot-rules/pii-in-logs.md` - ---- - -## K8. LIKE wildcard injection ב-prefix של משתמש - -**מקור:** 8-Projects C12 (Facebook-Leads-New — commit `2f45eca`) -**חומרה:** CRITICAL — מניפולציה של query / חשיפת נתונים - -### איך זה נראה -`get_config_by_prefix(prefix)` הריץ `WHERE key LIKE :prefix || '%'`. SQL `LIKE` מתייחס ל-`_` כ-"כל תו יחיד" ול-`%` כ-"כל רצף". prefix מהמשתמש כמו `test_key` תפס גם `test_key_foo` וגם `testXkey_foo` — ו-prefix של `%` היה תופס הכל. - -### כלל לזיהוי -לכל סעיף `LIKE` שנבנה מקלט משתמש: -1. ברח מ-`_` ומ-`%` (ומתו ה-escape עצמו) בקלט: `prefix.replace('\\','\\\\').replace('%','\\%').replace('_','\\_')` ושימוש ב-`LIKE :p ESCAPE '\\'`. -2. או שכתב להתאמה מדויקת (`=`) אם חיפוש prefix לא באמת נדרש. -3. SQLAlchemy: השתמש ב-`column.startswith(value, autoescape=True)` במקום `LIKE` ידני. - -### ראה גם -- `BY-STACK/postgres.md` -- `bugbot-rules/like-wildcard-injection.md` - ---- - -## K9. credential auth נשלח לפני storage (OTP / link / token) - -**מקור:** 8-Projects C4 (Shipment-bot — commits `552f0f7`, `155aa81`) -**חומרה:** HIGH — חוסר עקביות ב-lifecycle של auth, אפשרות לעקיפה דרך נתיב אימות חלופי - -### איך זה נראה -סדר הפעולות: -1. ייצור OTP. -2. שליחה דרך SMS / email (בלתי הפיך). -3. שמירה ב-Redis / DB. - -אם שלב 3 נכשל (Redis למטה, שגיאה זמנית), למשתמש יש קוד אמיתי ביד אבל אף verifier לא יוכל להתאים אותו. גרוע מכך, אם יש נתיב fallback "דלג על OTP" שמופעל על "Redis miss", התוקף שיכול לגרום ל-Redis להיות flaky מקבל עקיפה. - -### כלל לזיהוי -כל flow של "ייצור + שליחה של credential" חייב: -1. להתמיד (Redis SET / DB INSERT עם TTL) **לפני** השליחה החיצונית. -2. אם ההתמדה נכשלת, לא לשלוח. -3. אין נתיב "fallback" שממשיך כשאחסון verification לא בר השגה — fail closed. - -הכללה: כל triplet של "reserve / dispatch / store" שהפעולה החיצונית בו בלתי הפיכה — ראה CORE U1 (reserve-then-fill). - -### ראה גם -- `CORE-PATTERNS.md` U1 -- `BY-STACK/webhooks.md` -- `bugbot-rules/auth-before-irreversible-action.md` - ---- - -## K10. 500 גנרי מוצג כ-"invalid credentials" - -**מקור:** 8-Projects C57 (Web — commit `5196657`) -**חומרה:** HIGH — security UX + סיוע לאיתור משתמשים + מסתיר outages אמיתיים - -### איך זה נראה -Handler של login: -```js -catch (err) { - if (err.status === 500) showError("Invalid username or password"); - if (err.status === 401) showError("Invalid username or password"); -} -``` -כל כשל DB, רעש רשת, או באג ב-backend מופיע כ-"wrong password". שלוש תוצאות: -1. Outage אמיתי בלתי נראה ל-ops (משתמשים אומרים "הסיסמה שלי לא עובדת"). -2. עוזר לתוקפים שמחפשים enumeration של חשבונות (כל email נראה "שגוי"). -3. משתמשים נועלים את עצמם מחוץ לחשבונות אמיתיים בניסיון "לאפס" סיסמאות שכן עובדות. - -### כלל לזיהוי -טיפול בשגיאות login / auth חייב להסתעף: -- `401 / 403` → "Invalid credentials" (גנרי, לא דולף אם ה-email קיים). -- `5xx` → "Service temporarily unavailable. Please try again in a moment." + התראה למוניטורינג. -- `429` → "Too many attempts. Try again in N minutes." - -לעולם אל תאחד `5xx` ל-"invalid credentials". - -### ראה גם -- `bugbot-rules/auth-before-irreversible-action.md` (סעיף auth UX) - ---- - -## K11. כשל שמדווח בערך החזרה נבלע — הצלחה מדומה למשתמש - -**מקור:** CodeBot P1 (PR #3232, PR #3172, ומופע cache invalidation) — 3 מופעים באותו ריפו -**חומרה:** HIGH — אובדן נתונים שקט: המשתמש מקבל אישור על פעולה שלא קרתה - -### איך זה נראה -פונקציה מסמנת כשל בערך החזרה — `None`, `False`, `0`, `[]`, rowcount — ולא -בזריקת חריגה. הקורא עוטף אותה ב-`try/except`, מניח ש"לא נזרקה חריגה" פירושו -הצלחה, ומדווח ✅: - -```python -try: - saved = await asyncio.to_thread(backup_manager.save_backup_bytes, raw, metadata) - saved = True # save_backup_bytes מחזיר None בכשל — לא זורק -except Exception: - await notify_failure() # לא ירוץ לעולם -# המשתמש רואה "✅ נשמר גיבוי" על גיבוי שלא נשמר -``` - -הנזק הקשה הוא לא הכשל עצמו אלא **האישור השקרי**: משתמש שסומך על גיבוי -שאינו קיים, קאש שמוגש כמעודכן אחרי invalidation שמחק 0 מפתחות, "נשלח" -שלא נשלח. ה-try/except המיותר מחמיר — הוא משדר לקורא הבא "הכשל מטופל". - -### כלל לזיהוי -לכל קריאה שעטופה ב-try/except או שאחריה דיווח הצלחה: -1. בדוק את מסלול הכשל של הפונקציה הנקראת — זורקת או מחזירה falsy? (הקובע - הוא החוזה של הפונקציה, לא הערך: `0` מ-invalidation שאמור למחוק הוא - כשל, `0` ממחיקה אופורטוניסטית הוא תקין.) -2. אם מחזירה falsy בכשל: חובה `if not result:` אחרי הקריאה, לפני כל דיווח הצלחה. -3. דגל אדום: השמת דגל הצלחה קבוע (`saved = True`) מיד אחרי הקריאה. -4. בכתיבת פונקציה חדשה: בחר ערוץ כשל אחד — עדיף זריקה — ותעד אותו ב-docstring. - -### False positives -- פונקציות raise-on-error מתועדות — שם try/except הוא הערוץ הנכון. -- fire-and-forget מוצהר (מטריקות, לוגים) שכשל שקט מקובל בו. - -### מצב מומלץ -**strict** בכל נתיב שמסתיים בהודעת הצלחה למשתמש או בכתיבת נתונים. - -### ראה גם -- `bugbot-rules/return-value-failure-unchecked.md` -- `bugbot-rules/sdk-error-completeness.md` §3 — קרוב המשפחה בעולם ה-SDK: כשלים שחוזרים כערכים מ-`gather(return_exceptions=True)` -- `CORE-PATTERNS.md` U1 — בדיקת rowcount אחרי CAS היא מופע של אותו עקרון - ---- - -## K12. שאילתה רב-דיירית בלי tenant scope — דליפה בין לקוחות - -**מקור:** EmailFlow (3 מופעי auth/tenant isolation, HIGH — נספח "מחוץ ל-Top 7"); הקשר חי: ai-business-bot (ContextVar של tenant עם `default=None`) -**חומרה:** CRITICAL — דליפת נתונים בין לקוחות: הלקוח של דנה רואה את הלידים של יוסי - -### איך זה נראה -במערכת רב-דיירית, שאילתה אחת ששכחה `WHERE tenant_id = :current` מספיקה כדי שנתוני לקוח אחד יגיעו ללקוח אחר. הדליפה שקטה לגמרי — אין חריגה, אין לוג, התשובה "תקינה" רק עם נתונים של מישהו אחר. שלושת המסלולים הקלאסיים: - -1. **get-by-id בלי scope.** `SELECT ... WHERE id = :id` — ה-id הגיע מהלקוח, וכל מי שמנחש/מונה ids קורא שורות של דיירים אחרים (IDOR). -2. **נתיב משני שנשכח.** ה-list הראשי מסונן, אבל ה-export / החיפוש / ה-aggregation / ה-webhook handler — לא. הבידוד חזק בדיוק כמו השאילתה הכי חלשה. -3. **Context של tenant עם fallback שקט.** `ContextVar("tenant", default=None)` או `default=DEFAULT_TENANT` — נתיב ששכח לקבוע context לא נכשל, הוא רץ על הדייר הלא נכון. גרוע מדליפה: כתיבה לדייר הלא נכון. - -### כלל לזיהוי -1. חוזה לפי סוג הפעולה, בטבלה עם עמודת `tenant_id`/`account_id`/`org_id`: **SELECT / UPDATE / DELETE** — predicate על ה-tenant המאומת בכל שאילתה, כולל get-by-id, exports, aggregations, חיפוש ומחיקות. **INSERT / UPSERT** — אין WHERE שיציל אותך: ה-`tenant_id` הנכתב נגזר מה-context המאומת של הבקשה, לעולם לא מערך שהגיע מהלקוח; וב-UPSERT מפתח ה-conflict כולל את ה-tenant, אחרת דייר אחד דורס שורה של אחר. -2. עדיף אכיפה מרכזית על משמעת נקודתית: scoped session / query builder שמזריק את הסינון, RLS ב-Postgres, או repository שמקבל tenant כפרמטר חובה. -3. Context של tenant: **fail-closed** — בלי default; נתיב בלי context זורק, לא נופל בשקט לברירת מחדל. דגל `TENANCY_STRICT` כבוי בפרודקשן = הדפוס הזה בהמתנה. -4. מפתחות cache, session, וקבצים זמניים כוללים את ה-tenant — אחרת ה-cache מגיש נתוני דייר אחד לאחר. -5. בדיקת בידוד היא בדיקת חובה: שני tenants **סינתטיים** על מסד בדיקה זמני, פעולה זהה, ואימות ששום תשובה לא מכילה נתונים של השני. שלב ה-T2 — הרצה עם ההגנה כבויה כדי לוודא שהבדיקה נופלת — רץ **רק** ב-local או ב-CI על המסד הזמני; לעולם לא בפרודקשן, ב-staging, או בכל סביבה עם נתונים משותפים. - -### False positives -- טבלאות גלובליות באמת (קונפיג מערכת, קטלוג ציבורי) — מסומנות ככאלה במפורש. -- קוד admin שמוצהר ומאובטח ככזה (עם audit log), שסורק את כל הדיירים בכוונה. - -### מצב מומלץ -**strict** על כל טבלה עם עמודת tenant. אין warning-period לדפוס הזה — הדליפה הראשונה היא כבר אירוע אבטחה. - -### ראה גם -- `bugbot-rules/tenant-row-scoping.md` -- `CORE-PATTERNS.md` U5 — עדכון חלקי (הכתיבה מסוננת, הלוג לא) -- `bugbot-rules/privilege-escalation-unverified.md` — הקצה השני: מי בכלל מקבל להיות באיזה tenant - ---- - -## K13. סוד רוכב על מחרוזת נגזרת — דליפה ללוגים ול-Sentry דרך הודעות שגיאה - -**מקור:** CodeBot Pattern 5 (PR #3234, אוגוסט 2026) -**חומרה:** CRITICAL — חשיפת credential בערוצי תצפית, בלי שאף שורה "רושמת סוד" במפורש - -### איך זה נראה -- URL של Telegram Bot API (`.../bot/method`) הוצמד להודעת חריגה - (`msg += f" url={url}"`) ולשדה `e.url`. כל כשל רשת נשא את הטוקן המלא - ללוגים, ל-traceback ולאירועי Sentry. -- הניקוי שהיה קיים נכשל-פתוח: סריקה לפי שמות שדות שמפספסת את גוף - החריגה, `except: pass` שמחזיר את הערך הגולמי, וניקוי עמוק שמחזיר את - המקור בטיפוס לא מוכר. - -### כלל לזיהוי -1. כל בניית מחרוזת שמרכיבה סוד — f-string, `.format()`, `%`, שרשור, - URL builder — לעקוב לאן הערך זורם (source-to-sink). אם הוא מגיע - להודעת חריגה, לשדה על אובייקט חריגה, לכל שיטת לוג, או ל-API של - ניטור (capture_*, breadcrumbs) — חובה לנקות **בנקודת ההרכבה או - בהשמה**, לא אצל הצרכנים. -2. מנגנון ניקוי (redaction) חייב להיכשל-סגור: כשל המרה מחזיר placeholder; - כשל ב-before_send מפיל את האירוע; טיפוס לא מוכר לא חוזר גולמי אם - הייצוג הטקסטואלי שלו נושא סוד. -3. סריקה לפי שמות שדות (`if "token" in key`) אינה ניקוי — גוף חריגה, - breadcrumbs ו-repr של אובייקטים לא עוברים דרכה. סורקים את המבנה כולו. - -### מצב מומלץ -נקודת ניקוי מרכזית אחת; החריגה מנקה את שדותיה בהשמה; רשימת דפוסים אחת -ללוגים (הודעה + traceback); before_send נכשל-סגור עם אזהרה. - -### ראה גם -- `bugbot-rules/secret-in-derived-text.md` -- K6 — הקצה השני: סוד שדולף **ללקוח** ב-response; כאן הדליפה פנימית -- `bugbot-rules/side-effect-riding-on-log-line.md` — התאום ההפוך: שם סוד רוכב על טקסט נגזר ודולף החוצה, כאן פעולה רוכבת על טקסט לוג ונעלמת פנימה כשמנקים אותו. תיקון של K13 הוא הטריגר הנפוץ ביותר לדפוס ההוא. -- `docs/source-projects/codebot-patterns.md` Pattern 5 - - -## K14. סוד בשורת שאילתה — SDK הניטור מתעד אותו בעצמו, ורשימת הניקוי לא מכירה אותו - -**מקור:** CodeBot Pattern 8 (PR #3270, אוגוסט 2026) -**חומרה:** CRITICAL — credential של ספק חיצוני נכתב לערוץ תצפית בכל בקשה מוצלחת - -### איך זה נראה -- סוד מועבר כפרמטר URL — `params={"key": api_key}`, `?token=...` — צורה - שמופיעה בתיעוד של ספקים רבים. אף שורה בקוד לא רושמת אותו לשום מקום. -- אינטגרציית ה-HTTP של ה-SDK לניטור קוראת את הכתובת בעצמה ורושמת אותה. - ב-sentry-sdk **2.42.1 — הגרסה שנבדקה** — זה - `parse_url(str(request.url), sanitize=False)` ואז - `span.set_data(SPANDATA.HTTP_QUERY, parsed_url.query)` — לא מותנה - ב-`send_default_pii` ולא בשום דגל אחר. אותו צמד נמצא גם באינטגרציות - aiohttp ו-stdlib (שמכסה את requests/urllib) של אותה גרסה. -- **האינטגרציה אינה מופיעה בקונפיגורציה.** ב-2.42.1 היא ב- - `_AUTO_ENABLING_INTEGRATIONS` ונדלקת לבד כשהספרייה מותקנת; חיפוש - ברשימת ה-`integrations=[...]` שב-`init` לא ימצא אותה. **גרסאות אחרות - מחייבות אימות נפרד לפני שמסתמכים על ההתנהגות הזו.** -- הרישום קורה בכל בקשה **מוצלחת**, לא רק בכשל — בניגוד לדליפה דרך - הודעת חריגה, שדורשת תקלה. -- החצי השני: רשימת דפוסי הניקוי מזהה **צורות של סודות מוכרים** - (`ghp_`, `Bearer`, טוקן טלגרם). כל ספק חדש הוא דליפה חדשה עד שמישהו - נזכר להוסיף רג'קס. - -### כלל לזיהוי -1. סוד שנכנס לכתובת בקשה הוא ממצא כשמותקן SDK ניטור עם אינטגרציית - HTTP — גם אם הקוד שלך לא נוגע ב-URL אחרי הבנייה. לבדוק - `_AUTO_ENABLING_INTEGRATIONS`, לא רק את מה שנרשם ידנית. **דליפה - מאושרת** דורשת גם מסלול שליחה שלם — init עם DSN, transaction שנדגם, - והאירוע שורד את הניקוי; בלי אימות המסלול, לדווח כחשד/סיכון רדום. -2. רשימת דפוסי ניקוי שכולה דפוסי-צורה היא **רשימה שחורה**. חייב להיות - בה לפחות כלל אחד חסין-ספק — ניקוי לפי **שם** הפרמטר ולא לפי צורת - הערך. -3. כלל לפי שם חייב להתייחס לשם כמקטע: `auth_token`, `oauth_token`, - `id_token`, `private_key`, `x-api-key` הם שמות נפוצים, והתאמה מדויקת - מהמפריד מפספסת את כולם. **מקטע פירושו גבול משני צדדיו** — ושניהם - נדרשים: - - **לפני השם**: תחילת מחרוזת, `?`, `&`, או מפריד בתוך השם. בלי - הגבול הזה `?monkey=` נתפס, כי `monkey=` מכיל `key=` כתת-מחרוזת. - - **אחרי השם**: `=` מיד אחרי המילה הרגישה. בלי זה `?key_id=` ו- - `?token_type=` נתפסים, למרות שהם מזהים ולא סודות. - התחילית עצמה חייבת להסתיים באחד משלושה גבולות: מפריד (`_`, `-`, `.`), - גבול camelCase (`[a-z0-9]` ← `[A-Z]`), או מעבר ספרה ← אות - (`[0-9]` ← `[A-Za-z]`). בלי השני, `apiKey` ו-`clientSecret` מפוספסים; - בלי הספרה בצד השמאלי שלו, `v2Token` ו-`sha256Token` מפוספסים; ובלי - השלישי, `v2token` בכתיב קטן מפוספס — למרות שהוא נושא בדיוק את אותו - credential. **רצף ספרות הוא מפריד מקטע**, ולכן `v2` + `token` היא - קריאה טבעית ואילו `mon` + `key` אינה — וזה מה שמפריד את הגבול הזה - מגבול אות ← אות שהיה תופס את `?monkey=`. וגבול ה-camelCase חייב להיות רגיש-רישיות: בדפוס תחת - `(?i)`, `[A-Z]` תופס גם אותיות קטנות והגבול מתדרדר ל"בין כל שתי - אותיות", מה שמחזיר את `?monkey=`. -4. הכלל חייב לתפוס גם **מחרוזת שאילתה עירומה** (`key=...` בלי `?` - מוביל) — זו הצורה שבה שדות ניטור שומרים אותה. - -### מצב מומלץ -הסוד עובר ל-header ייעודי (`x-goog-api-key` אצל Google), ויושב על -**הלקוח** ולא באתר הקריאה כדי שאתר קריאה חדש יקבל אותו בלי לדעת עליו — -**ובלבד שכל היעדים של אותו לקוח הם של אותו ספק.** כותרת ברירת-מחדל -נשלחת בכל בקשה דרכו, ולקוח משותף שפונה גם ליעדים אחרים ישלח את -ה-credential למי שלא צריך אותו. לקוח ייעודי לספק, או בדיקת host מול -allowlist לפני הוספת הכותרת. - -רשימת דפוסי הניקוי משותפת בין כל הרשתות (Sentry + לוגים) ומכילה כלל -לפי-שם. אימות מול מבנה האירוע האמיתי, לא רק מול דוגמאות שכתבת. - -### ראה גם -- `bugbot-rules/secret-in-url-query.md` -- K13 — הצורה הבסיסית: הקוד שלך מרכיב ושולח ל-sink. כאן אין sink בקוד - שלך; ה-SDK קורא וכותב. "המצב המומלץ" של K13 היה מיושם במלואו ובכל - זאת נכשל, כי הוא אינו דורש שהרשימה תהיה חסינת-ספק. -- `bugbot-rules/secret-in-derived-text.md` — סעיף ה-false-positives שם - תוקן בעקבות הדפוס הזה. -- `docs/source-projects/codebot-patterns.md` Pattern 8 - ---- - -## K15. שומר שמתפרסם לפני הערך שהוא שומר עליו — השבתה שקטה עד ריסטארט - -**מקור:** CodeBot Pattern 11 (PR #3357, ספטמבר 2026) -**חומרה:** HIGH — תקלת רשת חולפת אחת משביתה שירות שלם בשקט, עד ריסטארט - -### איך זה נראה - -אתחול עצל של משאב משותף מחזיק **שני** משתנים — שומר וערך — והמסלול -המהיר בודק את השומר בזמן שהוא מחזיר את הערך: - -```python -def get_db(): - global client, db - if client is None: # ← השומר - with lock: - if client is None: - client = MongoClient(url) # פורסם ראשון - client.server_info() # ← סיבוב רשת שלם - db = client[name] # הערך, אחרון - return db # ← מה שבאמת מוחזר -``` - -שני מסלולי כשל. **בשניהם התסמין הנמשך הוא `None` ולא חריגה** — אבל לא באותו רגע: - -1. **מרוץ.** `server_info()` הוא סיבוב רשת. קורא מקביל שנכנס באמצעו רואה - שומר מאותחל וערך ריק. -2. **הרעלה קבועה — החמורה.** כאן ההבחנה חשובה: הקריאה **הראשונה - זורקת** כמצופה, וה-`except` עושה את עבודתו. אבל היא כבר הספיקה - להשאיר את `client` מוצב על לקוח שבור — ולכן **כל קריאה עתידית** מדלגת - על האתחול ומחזירה `None` לתמיד. הכשל הגלוי קורה פעם אחת; מה שנמשך - הוא השקט אחריו. אינה דורשת מקביליות בכלל. - -### למה זה HIGH ולא סתם באג מקביליות - -הכשל מגיע כ-`None`, ורוב הקוראים מסתעפים על "המשאב לא זמין" ומדרדרים -בחן. **המערכת מדווחת על עצמה כתקינה-חלקית בזמן שהמשאב בריא לגמרי.** אין -חריגה, אין התראה, והתסמין שמגיע למשתמש ("החיפוש לא מוצא") מצביע על -המקום הלא נכון. ב-CodeBot: 211 קריאות ל-`get_db()` בקוד הייצור, 96 מהן -בלי `try` צמוד. - -### כלל לזיהוי - -1. אתחול עצל שבו המשתנה הנבדק במסלול המהיר **אינו** המשתנה המוחזר. -2. פרסום לפני אימות: הצבה גלובלית ואחריה `ping`/`server_info`/`auth`. -3. כשל שמשאיר את השומר מוצב — הבדיקה: *אחרי החריגה, האם קריאה חוזרת - תנסה שוב?* -4. `double-checked locking` שבו הבדיקה החיצונית קוראת משתנה שאינו - האחרון שנכתב בתוך הנעילה. - -### המצב המומלץ - -לבדוק על **הערך המוחזר**, ולבנות את המשאב במשתנים מקומיים שמתפרסמים רק -כשהוא מוכן — הערך קודם, השומר אחריו. ב-CodeBot חמישה מתוך שישה אתרי -אתחול עצל כבר עשו זאת נכון (`if _collection is not None`); אחד סטה, והוא -זה שנקרא מ-211 מקומות. - -### אזהרה: התיקון לבדו הוא רגרסיה - -ההרעלה שבסעיף 2 שימשה גם כמפסק. הסרתה לבדה גררה את הקצה הנגדי — כל -קריאה מנסה מחדש ומשלמת timeout מלא. נמדד, 20 קריאות מול מארח שאינו -נפתר: **5.0 שניות** לפני, **111.0** אחרי התיקון הנאיבי, **5.1** עם מפסק -בעל זמן פתיחה **סופי**. ב-CI זה הרג עובד xdist שלם וארבע ריצות רצופות -נתקעו עד תקרת 30 הדקות. - -**הכלל שרחב מהדפוס: התנהגות מנוונת שקיימת מזמן היא לעיתים תשתית שמישהו -נשען עליה בלי שהצהיר. לפני שמסירים אותה — מודדים מי.** - -### ראה גם -- `bugbot-rules/lazy-init-guard-publish-order.md` -- `bugbot-rules/silent-fallback-to-worse-path.md` — הכשל כאן הוא מופע - שלו: המסלול החלופי ("המשך בלי מסד") עובד, רק גרוע, ואיש לא מדווח. -- `CORE-PATTERNS.md` U1 — משפחת המרוצים. שם הבעיה היא check-then-act על - מצב משותף; כאן היא **סדר הפרסום** של שני משתנים, וסעיף 2 אינו מרוץ - כלל. -- `docs/source-projects/codebot-patterns.md` Pattern 11 - ---- - -## K16. גבול נתיב שנבדק כקידומת מחרוזת — מחיקה מחוץ ל-allowlist - -**מקור:** CodeBot P13 (PR #3249, אוגוסט 2026) -**חומרה:** HIGH — `rmtree` על תיקייה שמחוץ לרשימה המותרת. בלתי הפיך, ונראה בקוד כמו בדיקת בטיחות תקינה - -### איך זה נראה - -השאלה היא "האם הנתיב הזה נמצא **בתוך** התיקייה המותרת". התשובה נכתבת כהשוואת **תווים**: - -```python -def safe_rmtree(p: Path, allow_under: Path): - if not str(p).startswith(str(allow_under)): # ❌ השוואת תווים - raise ValueError("outside allowlist") - shutil.rmtree(p) -``` - -עם `allow_under=/tmp/app-test`, המחרוזת `/tmp/app-test-evil` **עוברת** — היא באמת מתחילה באותם תווים, ואין ביניהן שום קשר היררכי. ‏`shutil.rmtree` מוחק תיקייה שלמה שמחוץ ל-allowlist. שוחזר אמפירית ב-CodeBot לפני התיקון: נתיב-אח נמחק בפועל. - -### התיקון - -```python -p, base = p.resolve(), allow_under.resolve() # ‏..‏ וקישורים סימבוליים קודם -if not (p == base or base in p.parents): # ✅ השוואת נתיב - raise ValueError("outside allowlist") -``` - -‏`parents` היא רשימת תיקיות, לא רצף תווים, ולכן היא אינה יכולה לענות "כן" ל-`-evil`. ה-`resolve()` אינו קישוט: בלעדיו `base/../../etc` עובר את אותה בדיקה. - -**וסייג שהתיקון הזה אינו פותר — TOCTOU:** בין ה-`resolve()` לבין ה-`rmtree` יש חלון. אם גורם לא מהימן יכול להחליף רכיב בנתיב באותו רגע (קישור סימבולי שמוחלף, תיקיית אב שמוזזת), הבדיקה עברה על נתיב אחד והמחיקה תתבצע על אחר. הכלל הזה מניח ש**העץ מהימן ואינו משתנה בזמן הפעולה** — ובסביבה שבה ההנחה אינה מתקיימת נדרש מנגנון אטומי (פתיחת מתאר לתיקייה ועבודה יחסית אליו, `O_NOFOLLOW`, או מרחב שמות ייעודי), לא בדיקת מחרוזת משופרת. - -### המשפחה המלאה — כל גבול היררכי שנבדק כמחרוזת - -| הדומיין | הצורה השבורה | מה עובר אותה | -|---|---|---| -| נתיב קבצים | `str(p).startswith(str(base))` | `/tmp/app-test-evil` | -| URL / origin | `url.startswith("https://api.example.com")` | `https://api.example.com.evil.net` | -| דומיין | `host.endswith("example.com")` | `notexample.com` | -| מפתח קאש / prefix | `key.startswith(f"user:{uid}")` | `user:12` תופס את `user:123` | - -בכל ארבעתם הצורה הנכונה היא להשוות עם המבנה שיודע מה הדומיין — `Path.parents`, גבול מפורש (`base + os.sep`, ‏`"." + domain`, ‏`prefix + ":"`), ו**לכתובות: פירוק ואז השוואה רכיב-רכיב**. - -**ודיוק על URL, כי `hostname` לבדו אינו origin:** ‏origin הוא **scheme + host + port אפקטיבי**. השוואת `urlsplit(u).hostname` לבדה מתאימה ל-allowlist של מארחים ותו לא — היא מקבלת `http://` במקום `https://`, ופורט אחר לגמרי. בכל מקום שבו ההחלטה היא origin — הפניה, בקשה יוצאת, בדיקת `Origin`/`Referer`, ‏`postMessage` — משווים את שלושת הרכיבים, עם הפורט המשתמע כשהוא חסר (443 ל-https, ‏80 ל-http). - -### כלל לזיהוי - -1. `startswith` / `endswith` / `in` על מחרוזת, כשהערך שנבדק הוא נתיב, URL, דומיין או מפתח מרחב-שמות — **ובמיוחד** כשהתוצאה שולטת בפעולה בלתי הפיכה (`rmtree`, ‏`unlink`, ‏`DROP`, הפניה, החלטת הרשאה). -2. בדיקת נתיב בלי `resolve()` / `realpath()` על **שני** הצדדים לפני ההשוואה. -3. השוואת קידומת בלי מפריד מפורש בסופה. - -### False positives - -- קידומת שהיא באמת קידומת טקסטואלית: שם קובץ שמתחיל ב-`tmp_`, בדיקת סכמה `url.startswith("https://")`, ניתוב לפי prefix בשכבת תצוגה. -- ‏`startswith` על ערך פנימי קבוע שלא מגיע מקלט ולא נגזר ממנו. - -### מצב מומלץ - -**strict** כשהתוצאה שולטת במחיקה, בכתיבה או בהחלטת הרשאה. **warning** אחרת. - -### ראה גם -- `bugbot-rules/path-prefix-not-boundary.md` -- `CRITICAL-PATTERNS.md` K8 — `LIKE` wildcard injection: גם שם הטעות היא טיפול בערך מובנה כמחרוזת שטוחה -- `docs/source-projects/codebot-history-scan-patterns.md` P13 diff --git a/tests/fixtures/md_corpus/INTEGRATION.md b/tests/fixtures/md_corpus/INTEGRATION.md deleted file mode 100644 index b66f4e5d2..000000000 --- a/tests/fixtures/md_corpus/INTEGRATION.md +++ /dev/null @@ -1,270 +0,0 @@ -# INTEGRATION — איך הידע מהריפו הזה מגיע לסשנים בזמן אמת - -הספרייה הזו שווה בדיוק כמה שהיא נקראת **בזמן המימוש**. המסמך הזה מגדיר -שלושה מנגנונים שמחברים אותה לעבודה השוטפת, ותהליך אחד שמונע ממנה להתיישן. - -**עקרון החלוקה:** `CLAUDE.md` של פרויקט מכיל *טריגרים* — מתי לקרוא ומה. -הריפו הזה מכיל את *הידע* — למה, איך מזהים, ומה false positive. לא מעתיקים -את הידע לפרויקטים (הוא מתיישן שם); מעתיקים רק את ההפניה. - ---- - -## 1. בלוק הטריגרים ל-CLAUDE.md (לכל פרויקט פעיל) - -מדביקים ב-`CLAUDE.md` של הפרויקט את התבנית שלמטה, ואת שורות הטבלה שבתוכה -ממלאים מ-**§2** לפי ה-stack של הפרויקט. שני החלקים יחד הם הבלוק — התבנית -לבדה חסרה את הטריגרים, והשורות לבדן חסרות את הוראת הקריאה. - -דרכי קריאה, לפי מה שזמין בסשן: mirror של הריפו הזה ב-CodeKeeper -(`codekeeper_search_repo` / `codekeeper_get_repo_file`), או ישירות -מ-GitHub (`amirbiron/amir-bug-patterns` הוא ריפו ציבורי). - -### תבנית הבלוק - -```markdown -## דפוסי באגים — amir-bug-patterns - -לפני שאתה נוגע באחד מהנושאים בטבלה — קרא את הקובץ המתאים ב-`amirbiron/amir-bug-patterns` (דרך MCP של CodeKeeper אם יש mirror, אחרת מ-GitHub). -אלה דפוסים שכבר עלו לי בפרודקשן — לא תיאוריה. - -| כשאתה נוגע ב... | קרא | -|---|---| -| <נושא> | <קובץ> | - - -תמיד, בכל פרויקט: -- לפני עטיפת קריאה ב-try/except → `CRITICAL-PATTERNS.md` K11 (כשל בערך החזרה) -- כותב `except` שבולע — `pass`, ערך falsy, או המשך כאילו הצליח → לוג, או הערה שאומרת למה הכשל הזה אינו מעניין. בלעדיהם זה ממצא, גם כשאין ✅ למשתמש במסלול. ‏(`raise` מחדש אינו בליעה) (K11, הצד ההפוך) -- לפני כתיבת טסט חדש → `claude-md-snippets/testing.md` -- אחרי שטסט נופל על חריגה → `bugbot-rules/widened-exception-scope.md` (אל תרחיב except) -- לפני העברת סוד כפרמטר URL (`params={"key": ...}`) → `CRITICAL-PATTERNS.md` K14. ה-SDK של הניטור רושם את השאילתה בעצמו, בכל בקשה מוצלחת. - -### סגירת הלולאה (חובה, לא רשות) -1. **ריוויוור (cubic/qodo/CodeRabbit/claude) תפס דפוס אמיתי** שאינו ב-amir-bug-patterns → פתח שם PR שמוסיף אותו (מסמך מקור + הצלבה לפי ה-README), **וגם** הוסף שורת טריגר לטבלה כאן. -2. **אתה בעצמך זיהית דפוס חוזר אצלך** (תיקנת פעמיים את אותו סוג טעות) → אותו תהליך בדיוק. -3. דפוס בלי שורת טריגר = דפוס שלא ייקרא. שני הצעדים הם צעד אחד. -4. **טריגר מזהה מה אתה מקליד, לא באיזה מצב אתה נמצא.** ‏`LOCK_FAIL_OPEN` ו-`0.0.0.0` רואים על המסך; "מסלול שרץ גם ברקע" ו"אחד מארבעת הנתיבים האלה" הם דברים שצריך **לדעת**. שורה שדורשת את הידע הזה כתנאי כניסה דורשת בדיוק את התשובה שהיא אמורה לתת, ולכן לא תידלק — התנאי שייך למסמך, לא לשורה. ומאותה סיבה: לתאר **מחלקה** ולא למנות מופעים, כי רשימה מתיישנת ברגע שנוסף המופע הבא. - -> [!IMPORTANT] -> **שורה ממופתחת לפי מה שיהיה מול העיניים, לא לפי הקטגוריה של הדפוס.** -> ‏U3 היה מתועד, והייתה לו שורה — `PyGithub / קריאות SDK חיצוני` — והיא לא -> נדלקה על ולידציה של ערך שהגיע מגוף JSON. הסיבה: השורה ממופתחת לפי מה -> שאתה **קורא לו**, והדפוס הוא על מאיפה ה**נתון** הגיע. רוב הדוגמאות של -> U3 אינן קריאות SDK בכלל. מי שכותב את הקוד מסתכל בטבלה, לא מוצא שורה, -> וממשיך. -> -> ולכן גם: **השורה מצביעה על הכלל הכללי ישירות.** הפניה לקובץ סטאק -> שמצליב לכלל היא קפיצה שנייה, ואותה עושים רק אחרי שכבר החלטת שהמקרה -> שלך בצורת אותו סטאק — כלומר בדיוק ההחלטה שהטבלה אמורה לחסוך. -> -> המבחן לפני שכותבים שורה: **האם מי שעומד לכתוב את הקוד יזהה את עצמו -> בניסוח הזה, לפני שהוא יודע מה הבאג?** אם לא — הניסוח לפי הקטגוריה, לא -> לפי המשימה. - -התהליך המלא והמיפוי לשאר הפרויקטים: `INTEGRATION.md` באותו ריפו. -``` - -### למה זה עובד -- `CLAUDE.md` נקרא בכל סשן ממילא — אין תלות בזיכרון של אף אחד. -- הטבלה קצרה (שורות בודדות) — לא מנפחת את הקובץ. -- הידע נשאר במקום אחד ומתעדכן במקום אחד. - -### והסניפטים האוניברסליים (universal / critical / testing)? - -ה-README (סעיף "איך להחיל על **פרויקט חדש**") אומר להדביק את שלושתם — -~70 שורות (עם `hebrew.md` — ~90, המספר שב-README). שני המסמכים לא סותרים, כי הם מדברים על שני מצבים: - -- **פרויקט חדש**: מדביקים את שלושת הסניפטים במלואם (צעד 1 ב-§4 כאן). - אין עדיין CLAUDE.md גדול, וה-70 שורות הן ה-baseline שכל השאר נבנה עליו. -- **פרויקט קיים עם CLAUDE.md מבוסס**: לא מדביקים אף אחד מהם במלואו — זה מנפח קובץ שכבר עמוס ויוצר עותק שמתיישן. מה כן, פר-סניפט: `testing.md` כבר מופנה מבלוק הטריגרים (שורת "לפני כתיבת טסט חדש") — ההפניה מספיקה; `universal.md` ו-`critical.md` נשארים בהפניה דרך הטריגרים של §2, ו**כלל בודד מתוכם שהדפוס שלו כבר עלה בפרויקט נפרס inline בבלוק** — כמו K11 ב-`CLAUDE.md` של CodeBot, שנכתב שם בגוף הטקסט כי הוא תפס את הריפו הזה שלוש פעמים. עלה דפוס נוסף בפרויקט ← מוסיפים גם את הכלל שלו inline, לא את כל הסניפט. - ---- - -## 2. מיפוי פר-פרויקט — מה ממלא את הטבלה שב-§1 - -> ⚠️ **מה שלמטה הוא רק שורות הטבלה, לא הבלוק להדבקה.** -> הטבלאות כאן נראות שלמות (יש להן כותרת ושורת מפריד) — אבל הן רק החלק -> שמשתנה בין פרויקטים. אם תדביק רק אותן, סוכן יראה `CRITICAL-PATTERNS.md` -> ולא יידע שזה ריפו אחר, ולא יקבל את הכללים הקבועים ולא את סגירת הלולאה. - -**איך מרכיבים, בשני צעדים:** - -1. קח את **התבנית המלאה מ-§1** — היא נותנת את המעטפת: הוראת הקריאה - (מאיפה מביאים את הקבצים), שלושת הכללים שחלים תמיד, וסגירת הלולאה. -2. החלף בה את שורת ה-placeholder `| <נושא> | <קובץ> |` בשורות של הפרויקט - שלך מהרשימה כאן — **רק שורות הנתונים**. אל תעתיק את שורת הכותרת - (`| כשאתה נוגע ב... | קרא |`) ואת שורת המפריד (`|---|---|`): הן כבר - קיימות בתבנית, והעתקה שלהן תיצור כותרת כפולה באמצע הטבלה. - -**דוגמה חיה, כבר מודבקת בפועל:** הסעיף *"דפוסי באגים — amir-bug-patterns"* -בסוף [`CLAUDE.md` של CodeBot](https://github.com/amirbiron/CodeBot/blob/main/CLAUDE.md). -שם רואים את התוצאה המורכבת — כולל התאמות מקומיות שהתבנית לא מכתיבה -(שורת "מתי להשתמש" בסגנון הריפו, וכלל K11 שנפרס בגוף הטקסט כי הוא זה -שכבר עלה שם שלוש פעמים). התבנית היא בסיס, לא חוזה נוקשה. - -השורות עצמן, לפי ה-stack של כל פרויקט: - -### CodeBot = CodeKeeper — ריפו אחד: `amirbiron/CodeBot` -(בוט טלגרם + webapp + MongoDB/GridFS + MCP + ChatOps + Sphinx docs. -CodeKeeper הוא שם המוצר, CodeBot הוא שם הריפו — לא שני פרויקטים.) - -| כשאתה נוגע ב... | קרא | -|---|---| -| שמירה/מחיקה שמסתיימת בהודעת ✅ למשתמש | `CRITICAL-PATTERNS.md` K11 | -| קאש / invalidation | `bugbot-rules/return-value-failure-unchecked.md` §4 | -| דגל שמצהיר "מטא-דאטה בלבד" (`npm install --package-lock-only`, `django-admin migrate --fake`), או רשומה שמתעדכנת בנפרד מהעבודה שהיא מתארת | `bugbot-rules/state-record-without-state-change.md` | -| callbacks / handlers מקביליים, מזהים מבוססי-זמן | `CORE-PATTERNS.md` U1 | -| `find_one_and_update` / `upsert` / תפיסת ג'וב | `CORE-PATTERNS.md` U1 + `bugbot-rules/race-toctou.md` | -| PyGithub / קריאות SDK חיצוני | `BY-STACK/external-sdk.md` | -| **ערך שהגיע מחוץ לתהליך** — גוף JSON, payload, כותרת, ENV, פלט של LLM, CSV, או ערך שמשתמש הקליד | `CORE-PATTERNS.md` U3 + `bugbot-rules/external-input-isinstance.md` | -| חיתוך או מדידת אורך של טקסט (`$substrBytes`, `$strLenBytes`, `encode()[a:b]`, תקרת אורך שנשלחת החוצה) | `BY-STACK/hebrew-source.md` H6 + `bugbot-rules/hebrew-source-and-data.md` §6 | -| הרכבת URL/מחרוזת שמכילה סוד, הודעות חריגה, ניקוי לוגים/Sentry | `CRITICAL-PATTERNS.md` K13 + `bugbot-rules/secret-in-derived-text.md` | -| קריאה ל-API עם מפתח/טוקן בפרמטרים, או שינוי ברשימת דפוסי הניקוי | `CRITICAL-PATTERNS.md` K14 + `bugbot-rules/secret-in-url-query.md` | -| מסיר שורת לוג, או עוטף אותה ב-guard שמונע הערכת ארגומנטים (הטריגר הנפוץ: תיקון PII) | `bugbot-rules/side-effect-riding-on-log-line.md` | -| גייטינג אדמין — רשימת היתר, התחזות (`/admin/impersonate/*`), או דגל שמרחיב הרשאה בהיעדר קונפיג (`CHATOPS_ALLOW_ALL_IF_NO_ADMINS`) | `CRITICAL-PATTERNS.md` K3 + `bugbot-rules/privilege-escalation-unverified.md`; לדגל עצמו — K12 סעיף 3 (fail-closed, בלי default שקט) | -| `LOCK_FAIL_OPEN` — מריץ polling בלי מנעול ה-singleton, כלומר שני מריצים במקום אחד | `CORE-PATTERNS.md` U1 | -| העלאת שרת HTTP או שינוי כתובת האזנה (`0.0.0.0`) | `CRITICAL-PATTERNS.md` K5 + `bugbot-rules/network-exposed-without-auth.md` | -| `docs/**/*.rst` | `bugbot-rules/line-number-coupling.md` | -| טסטים עם סטאבים ידניים | `TESTING-PATTERNS.md` + `bugbot-rules/widened-exception-scope.md` | -| אתחול עצל של משאב משותף (חיבור, לקוח, pool, קאש) — או **הסרה** של התנהגות מנוונת שקיימת מזמן | `CRITICAL-PATTERNS.md` K15 + `bugbot-rules/lazy-init-guard-publish-order.md` | -| `getattr(x, "y", None)` או `except` שאחריו **מסלול חלופי בגלל כשל** — לא ערך ברירת מחדל, ולא זיהוי יכולת סטטי | `bugbot-rules/silent-fallback-to-worse-path.md` | -| CSP, כותרות תגובה, או עמוד שנגיש בלי התחברות | `BY-STACK/browser-policy.md` | -| `create_index` — ובמיוחד `partialFilterExpression` או `sparse=True` | `BY-STACK/mongodb.md` דפוס 1 + `bugbot-rules/mongo-index-and-operator-traps.md` — אופרטורים שהפילטר החלקי לא מקבל, ואתחול שבולע את השגיאה | -| צינור `aggregate` — `$project`, `$sort`, `$group` — או `find_one` בתוך לולאה | `RECURRING-PATTERNS.md` R8 + `bugbot-rules/work-disproportionate-to-answer.md` | -| `startswith` / `endswith` על נתיב, URL או דומיין | `CRITICAL-PATTERNS.md` K16 | -| `replace(tzinfo=` · `datetime.now()` **בלי** אזור זמן · כל `date.today()` · הצגת תאריך למשתמש | `RECURRING-PATTERNS.md` R7 | -| **ברמה העליונה של מודול** (מחוץ לכל פונקציה): `Thread(`, `.start()`, `scheduler`, `asyncio.create_task`, לקוח או חיבור שנבנה מ-`os.environ`, או קריאת רשת, מסד או קובץ | `bugbot-rules/import-time-side-effects.md` | -| שינוי שנעשה כדי לספק לינטר: הזזת `import`, ניקוי אזהרת escape, הרחבת `except` | `bugbot-rules/linter-fix-changes-runtime-behavior.md` | -| מחיקה / שיתוף / שינוי שם לפי `_id` שהגיע מהממשק, ‏`created_at`, או פעולה גורפת על `code_snippets` ו-`large_files` | `bugbot-rules/logical-entity-vs-version-document.md` | -| שליחה לספק עם תקרת קלט (טוקנים, אורך שדה), או `value[:LIMIT]` לפני שמירה | `bugbot-rules/silent-truncation-at-sink.md` | -| `clientX` בחיסור, ‏`left`/`right`, גרירה, שינוי גודל, או הצמדה ל-viewport | `BY-STACK/hebrew-source.md` H7 + `bugbot-rules/rtl-geometry-and-clamp.md` | -| `EWMA` / `_ewma`, ‏`anomaly_detected`, ‏`cooldown`, ‏`first_ts`, ‏`adaptive_*_threshold_*` | `RECURRING-PATTERNS.md` R9 + `BY-STACK/observability.md` | -| הרצה ראשונה של ג'וב סנכרון: `baseline`, `last_*_check_time`, `if not last_seen` | `BY-STACK/cron-jobs.md` דפוס 10 — מה ההרצה הראשונה שולחת | -| תגית `