Document in-app file editing
Authored by
mfwolffe <wolffemf@dukes.jmu.edu>
- SHA
5ac31a3a8439c4b69e304efe69d0ba537542f550- Parents
-
0056ee6 - Tree
b08d8ce
5ac31a3
5ac31a3a8439c4b69e304efe69d0ba537542f5500056ee6
b08d8ce| Status | File | + | - |
|---|---|---|---|
| M |
docs/internal/branch-protection.md
|
9 | 3 |
| M |
docs/internal/code-tab.md
|
66 | 0 |
| M |
docs/internal/hooks.md
|
12 | 2 |
| M |
docs/internal/permissions.md
|
5 | 0 |
docs/internal/branch-protection.mdmodified@@ -70,7 +70,7 @@ Stored in `branch_protection_rules`: | |||
| 70 | | `pattern` | (set) | `filepath.Match` glob | | 70 | | `pattern` | (set) | `filepath.Match` glob | |
| 71 | | `prevent_force_push` | `true` | enforced | | 71 | | `prevent_force_push` | `true` | enforced | |
| 72 | | `prevent_deletion` | `true` | enforced | | 72 | | `prevent_deletion` | `true` | enforced | |
| 73 | -| `require_pr_for_push` | `false` | placeholder — column exists, no-op | | 73 | +| `require_pr_for_push` | `false` | enforced for direct git pushes and web edits | |
| 74 | | `allowed_pusher_user_ids` | `{}` | enforced; empty = no restriction | | 74 | | `allowed_pusher_user_ids` | `{}` | enforced; empty = no restriction | |
| 75 | | `require_signed_commits` | `false` | placeholder — post-MVP | | 75 | | `require_signed_commits` | `false` | placeholder — post-MVP | |
| 76 | | `status_checks_required` | `{}` | placeholder — S24 wires real checks | | 76 | | `status_checks_required` | `{}` | placeholder — S24 wires real checks | |
@@ -99,7 +99,8 @@ The pre-receive hook (`cmd/shithubd/hook.go::hookPreReceiveCmd`): | |||
| 99 | 3. For each ref update, calls `protection.Enforce`: | 99 | 3. For each ref update, calls `protection.Enforce`: |
| 100 | - Skip non-`refs/heads/*` refs (tag protection out of scope). | 100 | - Skip non-`refs/heads/*` refs (tag protection out of scope). |
| 101 | - Resolve the longest-matching rule. | 101 | - Resolve the longest-matching rule. |
| 102 | - - Apply the gates in order: deletion → force-push → allowed-pushers. | 102 | + - Apply the gates in order: deletion → require-PR → force-push → |
| 103 | + allowed-pushers. | ||
| 103 | - Return a `Decision` with `Allow`, `Reason`, and `Pattern`. | 104 | - Return a `Decision` with `Allow`, `Reason`, and `Pattern`. |
| 104 | 4. On any `Allow=false`: write `protection.FriendlyMessage(d)` to | 105 | 4. On any `Allow=false`: write `protection.FriendlyMessage(d)` to |
| 105 | stderr; exit non-zero. | 106 | stderr; exit non-zero. |
@@ -108,6 +109,12 @@ The pre-receive hook (`cmd/shithubd/hook.go::hookPreReceiveCmd`): | |||
| 108 | (via `repogit.IsAncestor`). When the old SHA is not an ancestor of | 109 | (via `repogit.IsAncestor`). When the old SHA is not an ancestor of |
| 109 | the new SHA, the update is non-fast-forward → reject. | 110 | the new SHA, the update is non-fast-forward → reject. |
| 110 | 111 | ||
| 112 | +**Require pull request** rejects direct branch creates and updates when | ||
| 113 | +`require_pr_for_push` is true on the matching rule. That covers both | ||
| 114 | +normal git pushes and in-browser file-editor commits because both paths | ||
| 115 | +call `protection.Enforce` before advancing `refs/heads/*`. Branch | ||
| 116 | +deletions remain controlled by `prevent_deletion`. | ||
| 117 | + | ||
| 111 | **Failure mode** (DB unavailable from hook): the rule lookup returns | 118 | **Failure mode** (DB unavailable from hook): the rule lookup returns |
| 112 | an error; the hook prints "transient; retry" to stderr and exits | 119 | an error; the hook prints "transient; retry" to stderr and exits |
| 113 | non-zero. **Fail closed**, per the S20 lean: better to reject a | 120 | non-zero. **Fail closed**, per the S20 lean: better to reject a |
@@ -157,7 +164,6 @@ land, the meta blob carries `action: "default_branch_changed"`/ | |||
| 157 | 164 | ||
| 158 | ## Deferred to later sprints | 165 | ## Deferred to later sprints |
| 159 | 166 | ||
| 160 | -- **`require_pr_for_push` enforcement** → S22 (PR engine) wires it. | ||
| 161 | - **`require_signed_commits` enforcement** → post-MVP signing surface. | 167 | - **`require_signed_commits` enforcement** → post-MVP signing surface. |
| 162 | - **`status_checks_required`** → S24 ships the check engine. | 168 | - **`status_checks_required`** → S24 ships the check engine. |
| 163 | - **Required reviewers attached to rules** → S23. | 169 | - **Required reviewers attached to rules** → S23. |
docs/internal/code-tab.mdmodified@@ -14,6 +14,15 @@ default branch Code tab directly, matching GitHub's canonical repo URL. | |||
| 14 | | `GET /{owner}/{repo}/blob/{ref}/{path...}` | `codeBlob` | | 14 | | `GET /{owner}/{repo}/blob/{ref}/{path...}` | `codeBlob` | |
| 15 | | `GET /{owner}/{repo}/raw/{ref}/{path...}` | `codeRaw` | | 15 | | `GET /{owner}/{repo}/raw/{ref}/{path...}` | `codeRaw` | |
| 16 | | `GET /{owner}/{repo}/find/{ref}?q=...` | `codeFinder` | | 16 | | `GET /{owner}/{repo}/find/{ref}?q=...` | `codeFinder` | |
| 17 | +| `GET /{owner}/{repo}/edit/{ref}/{path...}` | in-browser file editor | | ||
| 18 | +| `POST /{owner}/{repo}/edit/{ref}/{path...}` | commit file edit / rename | | ||
| 19 | +| `GET /{owner}/{repo}/new/{ref}/{path...}` | in-browser new-file editor | | ||
| 20 | +| `POST /{owner}/{repo}/new/{ref}/{path...}` | commit new file | | ||
| 21 | +| `GET /{owner}/{repo}/delete/{ref}/{path...}` | delete-file confirmation | | ||
| 22 | +| `POST /{owner}/{repo}/delete/{ref}/{path...}` | commit file deletion | | ||
| 23 | +| `GET /{owner}/{repo}/upload/{ref}/{path...}` | upload-files form | | ||
| 24 | +| `POST /{owner}/{repo}/upload/{ref}/{path...}` | commit uploaded files | | ||
| 25 | +| `POST /{owner}/{repo}/markdown-preview` | editor Markdown preview fragment | | ||
| 17 | | `GET /{owner}/{repo}/actions` | parked product-tab shell | | 26 | | `GET /{owner}/{repo}/actions` | parked product-tab shell | |
| 18 | | `GET /{owner}/{repo}/projects` | parked product-tab shell | | 27 | | `GET /{owner}/{repo}/projects` | parked product-tab shell | |
| 19 | | `GET /{owner}/{repo}/wiki` | parked product-tab shell | | 28 | | `GET /{owner}/{repo}/wiki` | parked product-tab shell | |
@@ -27,6 +36,10 @@ Every code-tab handler runs through `policy.Can(... ActionRepoRead)` — | |||
| 27 | private repos hide from anonymous viewers and unrelated users via the | 36 | private repos hide from anonymous viewers and unrelated users via the |
| 28 | existence-leak 404 guard from S15. | 37 | existence-leak 404 guard from S15. |
| 29 | 38 | ||
| 39 | +The in-browser mutation routes run through `policy.Can(... ActionRepoWrite)`. | ||
| 40 | +They inherit the same archived-repo, suspended-user, collaborator-role, | ||
| 41 | +site-admin, and private-repo existence behavior as git push surfaces. | ||
| 42 | + | ||
| 30 | ## Repository product tabs | 43 | ## Repository product tabs |
| 31 | 44 | ||
| 32 | The repo header intentionally exposes GitHub's major product-map tabs: | 45 | The repo header intentionally exposes GitHub's major product-map tabs: |
@@ -116,6 +129,56 @@ deferral path; the tree template has the column slot ready. | |||
| 116 | Chroma uses the `github` style baked at process start; the CSS is | 129 | Chroma uses the `github` style baked at process start; the CSS is |
| 117 | served from `/static/css/chroma.css` via a tiny in-process generator. | 130 | served from `/static/css/chroma.css` via a tiny in-process generator. |
| 118 | 131 | ||
| 132 | +## In-browser file edits | ||
| 133 | + | ||
| 134 | +The Code tab surfaces GitHub-style write affordances for users with | ||
| 135 | +`repo:write` on a named branch: | ||
| 136 | + | ||
| 137 | +- The tree header has an **Add file** dropdown with create and upload | ||
| 138 | + actions. | ||
| 139 | +- Text blob headers show edit and delete icon buttons. | ||
| 140 | +- The rendered README header shows an edit icon when the README was | ||
| 141 | + found in the current directory. SECURITY and CONTRIBUTING documents | ||
| 142 | + use the same blob-header controls when opened from the document tabs. | ||
| 143 | + | ||
| 144 | +Direct web commits are intentionally limited to `refs/heads/<branch>`. | ||
| 145 | +Tags and detached 40-hex commit views render read-only controls and | ||
| 146 | +direct edit URLs return `400`. | ||
| 147 | + | ||
| 148 | +`internal/repos/webedit` owns the mutation path. For each edit it: | ||
| 149 | + | ||
| 150 | +1. Resolves the branch to its current commit and compares the submitted | ||
| 151 | + hidden `base_oid`; a mismatch returns a stale-edit conflict. | ||
| 152 | +2. Builds a temporary index from the old commit with `git read-tree`. | ||
| 153 | +3. Stages file changes via canonical git plumbing (`hash-object`, | ||
| 154 | + `update-index`, `write-tree`, `commit-tree`). | ||
| 155 | +4. Runs `protection.Enforce` before moving the branch, so protected | ||
| 156 | + branches deny direct web commits just like pushes. | ||
| 157 | +5. Advances the branch with `git update-ref <ref> <new> <old>` CAS. | ||
| 158 | +6. Inserts a `push_events` row with `protocol = 'web'`, enqueues | ||
| 159 | + `push:process`, and sends the worker NOTIFY. If enqueueing fails | ||
| 160 | + after the ref has moved, the commit still succeeds and the failure | ||
| 161 | + is logged; the same post-push reconciliation gap exists for hook | ||
| 162 | + failures. | ||
| 163 | + | ||
| 164 | +Validation rules: | ||
| 165 | + | ||
| 166 | +- Text editor actions are capped at 1 MiB and reject NUL-byte binary | ||
| 167 | + content. Existing edit sources must be regular blobs, not symlinks, | ||
| 168 | + submodules, trees, or oversized blobs. | ||
| 169 | +- Uploads are capped at 25 MiB per request and 10 MiB per file. Uploads | ||
| 170 | + may contain binary data. | ||
| 171 | +- Repository paths reject empty names, leading/trailing slash, duplicate | ||
| 172 | + slash, backslash, `.`/`..` segments, control bytes, exact overwrites, | ||
| 173 | + duplicate uploads, and parent-path conflicts. | ||
| 174 | +- Default commit messages are generated server-side (`Update`, `Create`, | ||
| 175 | + `Rename`, `Delete`, or `Upload`) when the form leaves the message | ||
| 176 | + blank. | ||
| 177 | + | ||
| 178 | +The editor component is still server-rendered Go templates plus a small | ||
| 179 | +page-local script. No frontend build pipeline or React/Vite layer is | ||
| 180 | +required for this slice. | ||
| 181 | + | ||
| 119 | ## Raw view | 182 | ## Raw view |
| 120 | 183 | ||
| 121 | * Content-Type derived from the extension whitelist | 184 | * Content-Type derived from the extension whitelist |
@@ -168,6 +231,9 @@ mechanically straightforward. | |||
| 168 | (TODO — minimal coverage today). | 231 | (TODO — minimal coverage today). |
| 169 | * **Path traversal**: `validateSubpath` in `code.go` rejects `..`, | 232 | * **Path traversal**: `validateSubpath` in `code.go` rejects `..`, |
| 170 | controls, leading slashes. | 233 | controls, leading slashes. |
| 234 | +* **Web edit path traversal / overwrite**: `webedit.ValidateFilePath` | ||
| 235 | + applies the stricter mutation path guard and the service re-checks | ||
| 236 | + path existence against the commit being modified. | ||
| 171 | * **Hex collision with SHA**: ref-list lookup wins over SHA shortcut | 237 | * **Hex collision with SHA**: ref-list lookup wins over SHA shortcut |
| 172 | when the same string is both. | 238 | when the same string is both. |
| 173 | * **Encoding (GBK / Shift-JIS)**: TODO — text files outside UTF-8 may | 239 | * **Encoding (GBK / Shift-JIS)**: TODO — text files outside UTF-8 may |
docs/internal/hooks.mdmodified@@ -48,6 +48,14 @@ git push ──▶ receive-pack | |||
| 48 | worker picks it up via LISTEN | 48 | worker picks it up via LISTEN |
| 49 | ``` | 49 | ``` |
| 50 | 50 | ||
| 51 | +In-browser file-editor commits do not execute git hooks: they are built | ||
| 52 | +with plumbing inside the already-bare repository and advance the branch | ||
| 53 | +with `git update-ref`. To keep downstream behavior identical to pushed | ||
| 54 | +commits, `internal/repos/webedit` runs the same branch-protection | ||
| 55 | +enforcer before the ref update, inserts `push_events.protocol = 'web'` | ||
| 56 | +after a successful CAS, enqueues `push:process`, and sends | ||
| 57 | +`NOTIFY shithub_jobs`. | ||
| 58 | + | ||
| 51 | ## pre-receive contract | 59 | ## pre-receive contract |
| 52 | 60 | ||
| 53 | Implements the **minimum gates** described in S14. The full branch | 61 | Implements the **minimum gates** described in S14. The full branch |
@@ -66,8 +74,10 @@ to the pusher's terminal. Latency budget: <100ms p99. | |||
| 66 | ## post-receive contract | 74 | ## post-receive contract |
| 67 | 75 | ||
| 68 | * Reads stdin, ignores empty/malformed lines. | 76 | * Reads stdin, ignores empty/malformed lines. |
| 69 | -* For each `<old> <new> <ref>` line: INSERT a `push_events` row, then | 77 | +* For each `<old> <new> <ref>` line: INSERT a `push_events` row with |
| 70 | - enqueue a `push:process` job carrying the new event id. | 78 | + protocol `http` or `ssh`, then enqueue a `push:process` job carrying |
| 79 | + the new event id. Web-editor commits use the same table with protocol | ||
| 80 | + `web`. | ||
| 71 | * Issues a single `NOTIFY shithub_jobs` per push (workers wake on the | 81 | * Issues a single `NOTIFY shithub_jobs` per push (workers wake on the |
| 72 | next tx commit). | 82 | next tx commit). |
| 73 | * Exits 0 even on internal errors. The push has already landed; the | 83 | * Exits 0 even on internal errors. The push has already landed; the |
docs/internal/permissions.mdmodified@@ -74,6 +74,11 @@ The complete map (also enforced by the matrix test): | |||
| 74 | Read actions on **public** repos are short-circuited to allow before the | 74 | Read actions on **public** repos are short-circuited to allow before the |
| 75 | role check — anyone (anonymous or otherwise) can read a public repo. | 75 | role check — anyone (anonymous or otherwise) can read a public repo. |
| 76 | 76 | ||
| 77 | +The in-browser file editor uses `repo:write` for every mutation route | ||
| 78 | +(`edit`, `new`, `delete`, and `upload`). Its buttons are only rendered | ||
| 79 | +when the same action allows the current web actor on a named branch, and | ||
| 80 | +the POST handlers re-run the policy check before committing. | ||
| 81 | + | ||
| 77 | ## Decision precedence | 82 | ## Decision precedence |
| 78 | 83 | ||
| 79 | `Can()` evaluates in a fixed order; the first matching rule produces | 84 | `Can()` evaluates in a fixed order; the first matching rule produces |