Skip to content
This repository has been archived by the owner on Dec 13, 2023. It is now read-only.

Fix: #3876 Update task utils: removeIterationFromTaskRefName #3878

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

appunni-m
Copy link

Parsing name is not considering task ref name with double underscores

  • This is not fully fixing the use of this function. There needs to be some kind of validation against the user from setting up the taskRefName with double underscore

Pull Request type

  • Bugfix
  • Feature
  • Refactoring (no functional changes, no api changes)
  • Build related changes (Please run ./gradlew generateLock saveLock to refresh dependencies)
  • WHOSUSING.md
  • Other (please describe):

NOTE: Please remember to run ./gradlew spotlessApply to fix any format violations.

Changes in this PR

Describe the new behavior from this PR, and why it's needed
Issue #3876
If taskRefName with double underscore is not considered, then task creation would go into infinite loop. This can be used also as an attack vector on conductor installation as this causes DDos

Alternatives considered

Add validation in the create start workflow / create workflow definition api

Describe alternative implementation you have considered

Parsing name is not considering task ref name with double underscores

- This is not fully fixing the use of this function. There needs to be some kind of validation against the user from setting up the taskRefName with double underscore
Fix: Netflix#3876 Update task utils: removeIterationFromTaskRefName
@v1r3n
Copy link
Contributor

v1r3n commented Dec 4, 2023

Hi @appunni-m can you fix the build?

Added imports as required
@appunni-m
Copy link
Author

@v1r3n hey I have fixed the build.

@appunni-m
Copy link
Author

I am making one more change @v1r3n this will solve for #3880 as well

@manan164
Copy link
Contributor

manan164 commented Dec 4, 2023

Hi @appunni-m , Is there a particular reason that you are using double underscore in the task reference name?

@appunni-m
Copy link
Author

@manan164 i take it as input from user, thus I don't want to take chances with it. There are no validations against it either.

@appunni-m
Copy link
Author

@manan164 can we merge this ? Any blocker ?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants