Skip to content

Commit 8069dd1

Browse files
committed
fix(node): address beardthelion P3 findings on name bound and input messages
- Assert each rejection's own message in the reposPage invalid-inputs loop: the loop no longer accepts either message for every query, so a misattributed message (limit arm returning the cursor message) goes red. - Pin the create/fork name-bound wiring through the handler: new test drives create_repo with an overlong name via test_state_lazy and asserts the 400; deleting the validate_repo_name call site would proceed to the lazy DB pool and fail with a connection error instead. - Document the API-side vs disk-side validate_repo_name divergence on the new function, and tie the store's literal 100 to MAX_REPO_NAME_LEN so the two validators cannot drift apart on length.
1 parent 47070a9 commit 8069dd1

3 files changed

Lines changed: 69 additions & 14 deletions

File tree

‎crates/gitlawb-node/src/api/repos.rs‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -221,6 +221,16 @@ pub struct InfoRefsQuery {
221221
/// restricted, and the length is bounded so every cursor the server emits
222222
/// parses back: `repo_cursor` embeds the name in a base64(JSON) cursor that
223223
/// `parse_repo_cursor` rejects over 4096 bytes (#465 4th round).
224+
///
225+
/// This is the API-side validator, and it deliberately differs from the
226+
/// stricter disk-side `validate_repo_name` in `git/repo_store.rs`: this one
227+
/// admits names the store rejects (empty, leading `-`, non-ASCII
228+
/// alphanumeric) and rejects `.`, which the store allows. The divergence
229+
/// predates this bound (#412 owns the fork half), so a store-rejected name
230+
/// still fails later at `repo_store.init` after the proof is spent. Do not
231+
/// "fix" the divergence here; the length bound below is the one that keeps
232+
/// emitted cursors parseable, and it is shared via `MAX_REPO_NAME_LEN` so the
233+
/// two validators cannot drift apart on length.
224234
fn validate_repo_name(name: &str) -> Result<()> {
225235
// Sanitize name: alphanumeric, hyphens, underscores only
226236
if !name
@@ -3367,6 +3377,33 @@ mod tests {
33673377
assert!(validate_repo_name("").is_ok());
33683378
}
33693379

3380+
#[tokio::test]
3381+
async fn create_repo_rejects_overlong_name_through_handler() {
3382+
// Pins the wiring: `create_repo` must call `validate_repo_name`
3383+
// before any database access. The lazy state never connects, so
3384+
// deleting the call site would let the handler proceed to `get_repo`
3385+
// and fail with a connection error instead of BadRequest.
3386+
let state = crate::test_support::test_state_lazy();
3387+
let overlong = "a".repeat(crate::db::MAX_REPO_NAME_LEN + 1);
3388+
let err = create_repo(
3389+
State(state),
3390+
Extension(AuthenticatedDid(OWNER_DID.to_owned())),
3391+
axum::http::HeaderMap::new(),
3392+
Json(CreateRepoRequest {
3393+
name: overlong,
3394+
description: None,
3395+
is_public: true,
3396+
default_branch: "main".into(),
3397+
}),
3398+
)
3399+
.await
3400+
.unwrap_err();
3401+
assert!(
3402+
matches!(err, AppError::BadRequest(ref msg) if msg.contains("at most")),
3403+
"overlong name through create_repo must be a 400, got: {err:?}"
3404+
);
3405+
}
3406+
33703407
#[test]
33713408
fn upload_pack_request_finalizes_only_with_done_pktline() {
33723409
let want = "0032want 1111111111111111111111111111111111111111\n";

‎crates/gitlawb-node/src/git/repo_store.rs‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -625,8 +625,11 @@ fn validate_repo_name(repo_name: &str) -> Result<()> {
625625
if repo_name.is_empty() {
626626
anyhow::bail!("repo_name is empty");
627627
}
628-
if repo_name.len() > 100 {
629-
anyhow::bail!("repo_name exceeds 100 chars");
628+
// Byte bound shared with the API-side validator via `MAX_REPO_NAME_LEN`:
629+
// the two validators differ on charset by design, but must not drift on
630+
// length, or an API-admitted name fails here after the proof is spent.
631+
if repo_name.len() > crate::db::MAX_REPO_NAME_LEN {
632+
anyhow::bail!("repo_name exceeds {} chars", crate::db::MAX_REPO_NAME_LEN);
630633
}
631634
// Repo names are `[A-Za-z0-9._-]+` minus path-traversal traps.
632635
if repo_name.contains("..") {

‎crates/gitlawb-node/src/graphql/query.rs‎

Lines changed: 27 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -742,24 +742,39 @@ mod tests {
742742
.unwrap();
743743
let schema = schema(Arc::new(Db::for_testing(pool)));
744744
let over_max = crate::db::MAX_VISIBLE_REPO_PAGE_SIZE + 1;
745-
for query in [
746-
"{ reposPage(limit: 0) { hasNextPage } }".to_owned(),
747-
"{ reposPage(limit: -1) { hasNextPage } }".to_owned(),
748-
format!("{{ reposPage(limit: {over_max}) {{ hasNextPage }} }}"),
749-
"{ reposPage(after: \"invalid!\") { hasNextPage } }".to_owned(),
745+
let expected_limit_message = format!(
746+
"limit must be between 1 and {}",
747+
crate::db::MAX_VISIBLE_REPO_PAGE_SIZE
748+
);
749+
// Each input pins its own rejection message: the loop must not accept
750+
// either message for every query, or a misattributed message (e.g. the
751+
// limit arm returning "invalid repository cursor") stays invisible.
752+
for (query, expected_message) in [
753+
(
754+
"{ reposPage(limit: 0) { hasNextPage } }".to_owned(),
755+
expected_limit_message.clone(),
756+
),
757+
(
758+
"{ reposPage(limit: -1) { hasNextPage } }".to_owned(),
759+
expected_limit_message.clone(),
760+
),
761+
(
762+
format!("{{ reposPage(limit: {over_max}) {{ hasNextPage }} }}"),
763+
expected_limit_message.clone(),
764+
),
765+
(
766+
"{ reposPage(after: \"invalid!\") { hasNextPage } }".to_owned(),
767+
"invalid repository cursor".to_owned(),
768+
),
750769
] {
751770
let response =
752771
tokio::time::timeout(std::time::Duration::from_secs(1), anon(&schema, &query))
753772
.await
754773
.unwrap();
755774
assert_eq!(response.errors.len(), 1);
756-
let expected_limit_message = format!(
757-
"limit must be between 1 and {}",
758-
crate::db::MAX_VISIBLE_REPO_PAGE_SIZE
759-
);
760-
assert!(
761-
response.errors[0].message == expected_limit_message
762-
|| response.errors[0].message == "invalid repository cursor"
775+
assert_eq!(
776+
response.errors[0].message, expected_message,
777+
"wrong rejection message for query: {query}"
763778
);
764779
}
765780
// Valid base64 and valid cursor JSON: only the length guard rejects it.

0 commit comments

Comments
 (0)