Skip to content

fix(runway): validate GitHub merger config durations and hosts Copy title - #874

Closed
DivyanshGarg380 wants to merge 2 commits into
uber:mainfrom
DivyanshGarg380:DivyanshGarg380/fix-runway-github-config-validation
Closed

DivyanshGarg380 wants to merge 2 commits into
uber:mainfrom
DivyanshGarg380:DivyanshGarg380/fix-runway-github-config-validation

Conversation

@DivyanshGarg380

Copy link
Copy Markdown
Contributor

Why?

GitHub merger configuration currently accepts negative duration values and malformed host values. These configurations should be rejected early to prevent unexpected polling behavior and invalid host configuration.

What?

  • Reject negative values for httpTimeout, pollInterval, and maxPollDuration.
  • Reject GitHub merger hosts containing uppercase characters, schemes, or paths.
  • Preserve support for valid host:port values and default configuration.
  • Add regression tests covering valid and invalid durations and hosts.

Test Plan

  • go test ./service/runway/server -run TestLoadMergeConfig_GitHubDurationAndHostValidation -count=1 — passed.
  • gofmt -d service\runway\server\config.go service\runway\server\config_test.go — no formatting differences.
  • git diff --cached --check — passed.

Issue

@DivyanshGarg380
DivyanshGarg380 marked this pull request as draft October 16, 2026 11:42
@DivyanshGarg380
DivyanshGarg380 marked this pull request as ready for review October 16, 2026 11:42
Comment thread service/runway/server/config.go Outdated
if m.Host == "" {
m.Host = defaultGitHubHost
}
if m.Host != strings.ToLower(m.Host) || strings.ContainsAny(m.Host, "/ \t") {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think it should matter...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair point i will drop the uppercase check and keep the path validation.

@DivyanshGarg380

Copy link
Copy Markdown
Contributor Author

Hey, saw the PR was closed. was the change not needed or should i make some changes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants