Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(server): Reject invalid wf_run_id in RunWfRequest #1321

Merged
merged 17 commits into from
Feb 25, 2025

Conversation

Snarr
Copy link
Member

@Snarr Snarr commented Feb 20, 2025

Checks RunWfRequest wf_run_id against the LHUtil.isValidLHName() method in both the API layer and the process layer.

Resolves #1315

@Snarr Snarr changed the title fix(server): Reject invalid wf_spec_name in RunWfRequest fix(server): Reject invalid wf_run_id in RunWfRequest Feb 21, 2025
Copy link
Member

@coltmcnealy-lh coltmcnealy-lh left a comment

Choose a reason for hiding this comment

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

Pre-emptively approving with a comment to address

@Snarr Snarr requested a review from eduwercamacaro February 24, 2025 19:11
@Snarr
Copy link
Member Author

Snarr commented Feb 24, 2025

@eduwercamacaro Could you take a look at this and let me know what you think of the test?

throw new LHApiException(Status.INVALID_ARGUMENT, "Missing required argument 'wf_spec_name'");
}

if (id != null) {
Copy link
Member

@sauljabin sauljabin Feb 24, 2025

Choose a reason for hiding this comment

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

if id is an attribute you can make a method this.isIdValid()

also yo can use Strings.isNullOrEmpty

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, I implemented the isIdValid method.

I am not sure if Strings.isNullOrEmpty would apply here, since the id can be null but shouldn't be an empty string. Null implies that the id wasn't set, but empty string implies that someone tried to set the id with no value.

@Snarr Snarr self-assigned this Feb 24, 2025
@Snarr Snarr added bug An issue with correctness, stability, performance, or API conformance. server Affects the core LH Server code. labels Feb 24, 2025
@Snarr Snarr merged commit f4c0850 into master Feb 25, 2025
24 checks passed
@Snarr Snarr deleted the fix/reject-invalid-run-wf-spec-name branch February 25, 2025 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue with correctness, stability, performance, or API conformance. server Affects the core LH Server code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reject RunWf requests with underscores in the WfRunId
3 participants