From 0cd069fc74e573a559cbba6c12e1c8b5cf46cf6c Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Wed, 15 Jul 2026 15:30:55 +0200 Subject: [PATCH] fix(gitlab): stop connection test/search from firing thousands of requests (#9034) (#9051) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(gitlab): avoid request storm on connection test and search (#9034) searchIssueInProject$ paginated through every matching issue and then fired a paginated /notes request per issue via forkJoin. With an empty search term (used by testConnection) or a broad one, a project with thousands of issues produced thousands of rapid requests, which GitLab rate-limited with a 429 and surfaced as "connection failed". The fetched comments were never used: search results only need id + title, the comment count already rides along via user_notes_count, and full notes are re-fetched fresh in the detail view via getById$. Fetch only the first page for search (results are already ordered by updated_at) and drop the per-issue comment fan-out. This turns the connection test and each search from thousands of requests into one. Add gitlab-api.service.spec.ts asserting search issues a single first-page request, never follows x-next-page, and never requests notes. * fix(gitlab): make project pattern valid as a native input attribute (#9034) Formly writes templateOptions.pattern verbatim to the native attribute, and Chromium 146 compiles that attribute with the RegExp `v` flag. The field passed a RegExp object, so its `/…/i` stringification became the attribute value, and the unescaped `/` and `-` in the `[\w.%/-]` character class are reserved under `v` — Chromium logged "Invalid regular expression: Invalid character in character class" on every change-detection cycle (hundreds of times during the request storm from the connection test). Pass the regex `.source` string instead, escape `/` and `-` in the class (`[\w.%\/\-]`), and make the pattern flag-independent by writing the only case-sensitive literal as `%2[Ff]` so dropping the `i` flag preserves the lowercase `%2f` separator. Matching behaviour is unchanged (verified against all existing valid/invalid cases). Angular's Validators.pattern still enforces the same rule via the source string. Add a regression test that the source compiles under the `v` flag. --- .../gitlab-api/gitlab-api.service.spec.ts | 140 ++++++++++++++++++ .../gitlab/gitlab-api/gitlab-api.service.ts | 40 +++-- .../gitlab/gitlab-cfg-form.const.spec.ts | 32 ++-- .../providers/gitlab/gitlab-cfg-form.const.ts | 16 +- 4 files changed, 200 insertions(+), 28 deletions(-) create mode 100644 src/app/features/issue/providers/gitlab/gitlab-api/gitlab-api.service.spec.ts diff --git a/src/app/features/issue/providers/gitlab/gitlab-api/gitlab-api.service.spec.ts b/src/app/features/issue/providers/gitlab/gitlab-api/gitlab-api.service.spec.ts new file mode 100644 index 0000000000..801172b512 --- /dev/null +++ b/src/app/features/issue/providers/gitlab/gitlab-api/gitlab-api.service.spec.ts @@ -0,0 +1,140 @@ +import { + HttpClientTestingModule, + HttpTestingController, +} from '@angular/common/http/testing'; +import { TestBed } from '@angular/core/testing'; +import { SnackService } from 'src/app/core/snack/snack.service'; +import { GitlabApiService } from './gitlab-api.service'; +import { GitlabCfg } from '../gitlab.model'; +import { DEFAULT_GITLAB_CFG } from '../gitlab.const'; +import { GitlabOriginalIssue, GitlabOriginalUser } from './gitlab-api-responses'; +import { SearchResultItem } from '../../../issue.model'; + +const USER: GitlabOriginalUser = { + id: 1, + username: 'u', + name: 'U', + state: 'active', + avatar_url: '', + web_url: '', +}; + +const makeOriginalIssue = (iid: number): GitlabOriginalIssue => ({ + id: 1000 + iid, + iid, + project_id: 1, + title: `Issue ${iid}`, + description: 'desc', + state: 'open', + weight: 0, + created_at: '2026-01-01T00:00:00.000Z', + updated_at: '2026-01-02T00:00:00.000Z', + closed_at: '', + closed_by: '', + labels: [], + milestone: null, + assignees: [], + author: USER, + assignee: USER, + user_notes_count: 7, + merge_requests_count: 0, + upvotes: 0, + downvotes: 0, + due_date: '', + confidential: false, + discussion_locked: false, + web_url: `https://gitlab.com/group/sub/proj/-/issues/${iid}`, + time_stats: { + time_estimate: 0, + total_time_spent: 0, + human_time_estimate: '', + human_total_time_spent: '', + }, + task_completion_status: { count: 0, completed_count: 0 }, + has_tasks: false, + task_status: '', + _links: { + self: `https://gitlab.com/api/v4/projects/1/issues/${iid}`, + notes: `https://gitlab.com/api/v4/projects/1/issues/${iid}/notes`, + award_emoji: '', + project: '', + }, + references: { + short: `#${iid}`, + relative: `#${iid}`, + full: `group/sub/proj#${iid}`, + }, + moved_to_id: 0, +}); + +describe('GitlabApiService', () => { + let service: GitlabApiService; + let httpMock: HttpTestingController; + + const cfg: GitlabCfg = { + ...DEFAULT_GITLAB_CFG, + isEnabled: true, + project: 'group/sub/proj', + token: 'token', + scope: 'all', + }; + + beforeEach(() => { + TestBed.configureTestingModule({ + imports: [HttpClientTestingModule], + providers: [ + GitlabApiService, + { + provide: SnackService, + useValue: jasmine.createSpyObj('SnackService', ['open']), + }, + ], + }); + service = TestBed.inject(GitlabApiService); + httpMock = TestBed.inject(HttpTestingController); + }); + + afterEach(() => httpMock.verify()); + + describe('searchIssueInProject$', () => { + it('fetches only the first page and never requests per-issue notes/comments (#9034)', () => { + let result: SearchResultItem[] | undefined; + service.searchIssueInProject$('bug', cfg).subscribe((r) => (result = r)); + + const reqs = httpMock.match(() => true); + expect(reqs.length).toBe(1); + const req = reqs[0].request; + expect(req.method).toBe('GET'); + expect(req.url).toContain('/issues'); + expect(req.url).toContain('page=1'); + expect(req.url).toContain('per_page=100'); + expect(req.url).not.toContain('/notes'); + + // Respond WITH an x-next-page header — the service must NOT follow it, otherwise a + // large project would trigger the request storm this fix removes. + reqs[0].flush([makeOriginalIssue(1), makeOriginalIssue(2)], { + // eslint-disable-next-line @typescript-eslint/naming-convention + headers: { 'x-next-page': '2' }, + }); + + // No follow-up page request and no per-issue /notes request. + httpMock.expectNone(() => true); + expect(result?.length).toBe(2); + expect(result?.[0].title).toBe('#group/sub/proj#1 Issue 1'); + expect(result?.[0].issueType).toBe('GITLAB'); + }); + + it('resolves to [] and sends no request when settings are invalid', () => { + let result: SearchResultItem[] | undefined; + let completed = false; + service + .searchIssueInProject$('bug', { ...cfg, project: null }) + .subscribe({ next: (r) => (result = r), complete: () => (completed = true) }); + + httpMock.expectNone(() => true); + // EMPTY completes without emitting a value. + expect(result).toBeUndefined(); + expect(completed).toBe(true); + }); + }); +}); diff --git a/src/app/features/issue/providers/gitlab/gitlab-api/gitlab-api.service.ts b/src/app/features/issue/providers/gitlab/gitlab-api/gitlab-api.service.ts index a2fd0fb3e1..967514974f 100644 --- a/src/app/features/issue/providers/gitlab/gitlab-api/gitlab-api.service.ts +++ b/src/app/features/issue/providers/gitlab/gitlab-api/gitlab-api.service.ts @@ -7,7 +7,7 @@ import { HttpRequest, } from '@angular/common/http'; import { parseUrl, stringifyUrl } from 'query-string'; -import { EMPTY, forkJoin, Observable, of } from 'rxjs'; +import { EMPTY, Observable } from 'rxjs'; import { SnackService } from 'src/app/core/snack/snack.service'; import { GitlabCfg } from '../gitlab.model'; @@ -118,27 +118,20 @@ export class GitlabApiService { if (!this._isValidSettings(cfg)) { return EMPTY; } - return this._sendIssuePaginatedRequest$( + // NOTE: only the first page is fetched and issue comments are intentionally NOT + // loaded here. The search dropdown only needs id + title (the comment count already + // rides along via user_notes_count, and full notes are re-fetched in the detail view + // via getById$). A broad or empty search term can match thousands of issues, so + // paginating through all of them and then firing a /notes request per issue produced a + // burst of thousands of requests that GitLab rate-limited with a 429 — see #9034. + return this._sendIssueRequestFirstPage$( { url: `${this._apiLink(cfg)}/issues?search=${searchText}${this.getScopeParam( cfg, )}&order_by=updated_at${this.getCustomFilterParam(cfg)}`, }, cfg, - ).pipe( - mergeMap((issues: GitlabIssue[]) => { - if (issues && issues.length) { - return forkJoin([ - ...issues.map((issue) => this.getIssueWithComments$(issue, cfg)), - ]); - } else { - return of([]); - } - }), - map((issues: GitlabIssue[]) => { - return issues ? issues.map(mapGitlabIssueToSearchResult) : []; - }), - ); + ).pipe(map((issues) => issues.map(mapGitlabIssueToSearchResult))); } getProjectIssues$(cfg: GitlabCfg): Observable { @@ -241,6 +234,21 @@ export class GitlabApiService { ); } + // Fetches only the first page of issues (no x-next-page follow-up) and maps the raw + // response body straight to GitlabIssue[]. Used for search, where fetching every page is + // both unnecessary and a rate-limit risk on large projects (see searchIssueInProject$). + private _sendIssueRequestFirstPage$( + params: HttpRequest | any, + cfg: GitlabCfg, + ): Observable { + return this._sendPaginatedRequestImpl$(params, cfg, 1).pipe( + map((res: any) => { + const issues: GitlabOriginalIssue[] = res && res.body ? res.body : []; + return issues.map((issue) => mapGitlabIssue(issue, cfg)); + }), + ); + } + private _sendPaginatedRequest$( params: HttpRequest | any, cfg: GitlabCfg, diff --git a/src/app/features/issue/providers/gitlab/gitlab-cfg-form.const.spec.ts b/src/app/features/issue/providers/gitlab/gitlab-cfg-form.const.spec.ts index 2d9a44c55d..37dcc0e3bd 100644 --- a/src/app/features/issue/providers/gitlab/gitlab-cfg-form.const.spec.ts +++ b/src/app/features/issue/providers/gitlab/gitlab-cfg-form.const.spec.ts @@ -4,22 +4,34 @@ import { } from './gitlab-cfg-form.const'; describe('GITLAB_PROJECT_REGEX', () => { - let projectPattern: RegExp; - beforeAll(() => { - // Verify the form field is actually wired to the exported regex, so this - // spec fails if someone swaps the field's `pattern` out from under it. + // The form field is wired to the regex SOURCE STRING (not the RegExp object), + // because Formly writes it verbatim to the native `` attribute. + // Fail here if someone swaps the field's `pattern` out from under it. const projectField = GITLAB_CONFIG_FORM_SECTION.items!.find( (item) => item.key === 'project', ); - const pattern = projectField?.templateOptions?.pattern as RegExp; - expect(pattern).toBe(GITLAB_PROJECT_REGEX); - projectPattern = pattern; + const pattern = projectField?.templateOptions?.pattern; + expect(pattern).toBe(GITLAB_PROJECT_REGEX.source); }); - // Angular's Validators.pattern uses `regex.test(value)` as-is for a RegExp - // (no auto-anchoring), so the regex itself must be anchored at both ends. - const isValid = (value: string): boolean => projectPattern.test(value); + // Angular's Validators.pattern with a string wraps it in `^(?:…)$`; the source is + // already anchored, so it is used as-is. Testing the exported RegExp is equivalent. + const isValid = (value: string): boolean => GITLAB_PROJECT_REGEX.test(value); + + // Regression guard for #9034: the source is written to the native `` + // attribute, which Chromium compiles with the RegExp `v` flag. A value that is not + // `v`-safe throws "Invalid regular expression" on every change-detection cycle. + it('compiles as a native pattern attribute (RegExp `v` flag)', () => { + expect(() => new RegExp(`^(?:${GITLAB_PROJECT_REGEX.source})$`, 'v')).not.toThrow(); + }); + + it('is flag-independent so it needs no `i` flag on the attribute', () => { + expect(GITLAB_PROJECT_REGEX.flags).toBe(''); + // The only case-sensitive literal is the encoded separator; both cases accepted. + expect(isValid('group%2Fproject')).toBe(true); + expect(isValid('group%2fproject')).toBe(true); + }); describe('valid project identifiers', () => { const validCases = [ diff --git a/src/app/features/issue/providers/gitlab/gitlab-cfg-form.const.ts b/src/app/features/issue/providers/gitlab/gitlab-cfg-form.const.ts index 1be44f9758..70ef0c5459 100644 --- a/src/app/features/issue/providers/gitlab/gitlab-cfg-form.const.ts +++ b/src/app/features/issue/providers/gitlab/gitlab-cfg-form.const.ts @@ -16,7 +16,16 @@ import { // about the segment chars (e.g. consecutive hyphens, which GitLab paths allow) to // avoid false-rejecting valid paths; the separator lookahead keeps the char class a // single unnested quantifier (no catastrophic backtracking). -export const GITLAB_PROJECT_REGEX = /^(?:[1-9][0-9]*|(?=.*(?:\/|%2F))[\w.%/-]+)$/i; +// +// This pattern's `.source` is handed to Formly's `pattern` option, which both feeds +// Angular's `Validators.pattern` AND is written verbatim to the native `` +// attribute. Chromium compiles that attribute with the RegExp `v` flag, under which `/` +// and `-` are reserved inside a character class and MUST be escaped — hence `[\w.%\/\-]`. +// It must also stay flag-independent (the attribute cannot carry an `i` flag, and a +// stringified `/…/i` RegExp is not a valid attribute value), so the only case-sensitive +// literal, the encoded separator, is written explicitly as `%2[Ff]`. Getting this wrong +// makes Chromium log "Invalid regular expression" on every change-detection cycle (#9034). +export const GITLAB_PROJECT_REGEX = /^(?:[1-9][0-9]*|(?=.*(?:\/|%2[Ff]))[\w.%\/\-]+)$/; export const GITLAB_CONFIG_FORM: LimitedFormlyFieldConfig[] = [ ...CROSS_ORIGIN_WARNING, @@ -27,7 +36,10 @@ export const GITLAB_CONFIG_FORM: LimitedFormlyFieldConfig[] required: true, label: T.F.GITLAB.FORM.PROJECT, type: 'text', - pattern: GITLAB_PROJECT_REGEX, + // Pass the source string (not the RegExp object): Formly writes this straight to the + // native `` attribute, and a stringified `/…/` RegExp is not a valid + // attribute value. See GITLAB_PROJECT_REGEX for the `v`-flag constraints (#9034). + pattern: GITLAB_PROJECT_REGEX.source, description: T.F.GITLAB.FORM.PROJECT_HINT, }, },