Skip to content

[Improvement-17994][Seatunnel] harden startupScript and -i args#17996

Merged
fuchanghai merged 5 commits intoapache:devfrom
yzeng1618:dev-update
Feb 28, 2026
Merged

[Improvement-17994][Seatunnel] harden startupScript and -i args#17996
fuchanghai merged 5 commits intoapache:devfrom
yzeng1618:dev-update

Conversation

@yzeng1618
Copy link
Contributor

Purpose of the pull request

Harden startup script validation and fix -i variable quoting issues for the SeaTunnel task to prevent path traversal and shell injection. Also update outdated SeaTunnel task documentation.

Brief change log

  • Add startupScript allowlist validation in SeatunnelParameters#checkParameters

  • Add bash-safe quoting/escaping for -i variable values in SeatunnelTask

  • Support values containing single quotes ' in -i parameters

Update SeaTunnel task docs (EN/ZH): refresh links to 2.3.12

Verify this pull request

This pull request is already covered by existing tests, such as:

  • Unit tests in dolphinscheduler-task-plugin/dolphinscheduler-task-seatunnel: org.apache.dolphinscheduler.plugin.task.seatunnel.SeatunnelTaskTest (covers buildOptions() including config suffix detection, reading config from Resource Center, and -i parameter generation).

Pull Request Notice

Pull Request Notice

If your pull request contains incompatible change, you should also add it to docs/docs/en/guide/upgrade/incompatible.md

This PR does not contain incompatible changes.

Fix #17994

@boring-cyborg
Copy link

boring-cyborg bot commented Feb 25, 2026

Thanks for opening this pull request! Please check out our contributing guidelines. (https://github.com/apache/dolphinscheduler/blob/dev/docs/docs/en/contribute/join/pull-request.md)

@SbloodyS SbloodyS added the first time contributor First-time contributor label Feb 25, 2026
@SbloodyS SbloodyS added this to the 3.4.1 milestone Feb 25, 2026
@SbloodyS SbloodyS added the improvement make more easy to user or prompt friendly label Feb 25, 2026
- Deployment mode: specify the deployment mode, `cluster` `local`

> Click [here](https://seatunnel.apache.org/docs/2.3.3/command/usage) for more information on the usage of Apache SeaTunnel command`
> Click [here](https://seatunnel.apache.org/docs/2.3.12/command/usage) for more information on the usage of Apache SeaTunnel command`
Copy link
Member

Choose a reason for hiding this comment

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

Why change this? We support v2.3.3 at most for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that versions above SeaTunnel 2.3.3 should be supported for now. So I'll update it to the latest version 2.3.12. Please correct me if my understanding is wrong. Alternatively, I can temporarily revert it back to the original version.

Copy link
Member

Choose a reason for hiding this comment

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

Please revert this since we don't didn't conduct a comprehensive compatibility test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, it has been changed to 2.3.3.

@yzeng1618 yzeng1618 requested a review from SbloodyS February 26, 2026 03:06
@yzeng1618
Copy link
Contributor Author

@SbloodyS Could you please help check the cause of the CI error? It seems the error isn’t related to my changes. Shall we retry it?

@SbloodyS
Copy link
Member

@SbloodyS Could you please help check the cause of the CI error? It seems the error isn’t related to my changes. Shall we retry it?

I've restarted CI.

SbloodyS
SbloodyS previously approved these changes Feb 27, 2026
Copy link
Member

@SbloodyS SbloodyS left a comment

Choose a reason for hiding this comment

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

LGTM

@SbloodyS SbloodyS requested a review from ruanwenjun February 27, 2026 01:50
fuchanghai
fuchanghai previously approved these changes Feb 27, 2026
Copy link
Member

@fuchanghai fuchanghai left a comment

Choose a reason for hiding this comment

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

LGTM

&& resourceList.size() == 1));
}

private static boolean isValidStartupScript(String startupScript) {
Copy link
Member

@fuchanghai fuchanghai Feb 27, 2026

Choose a reason for hiding this comment

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

should we add a UT to validate this method? cc @SbloodyS

example

  @Test
  public void testInvalidStartupScript() {
      assertFalse(isValidStartupScript("../../../etc/passwd"));
      assertFalse(isValidStartupScript("script.sh; rm -rf /"));
      assertFalse(isValidStartupScript("script"));  // no suffix
      assertFalse(isValidStartupScript(".sh"));     // no file name
  }

  @Test
  public void testQuoteForBash() {
      assertEquals("'value'", quoteForBash("value"));
      assertEquals("'abc'"'"'def'", quoteForBash("abc'def"));
      assertEquals("''", quoteForBash(null));
      assertEquals("'$(rm -rf /)'", quoteForBash("$(rm -rf /)"));
  }

Copy link
Contributor Author

@yzeng1618 yzeng1618 Feb 27, 2026

Choose a reason for hiding this comment

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

Thank you for your suggestion. It has been added.

@yzeng1618 yzeng1618 dismissed stale reviews from SbloodyS and fuchanghai via b9a4469 February 27, 2026 06:04
@github-actions github-actions bot added the test label Feb 27, 2026
@yzeng1618
Copy link
Contributor Author

yzeng1618 commented Feb 27, 2026

@SbloodyS Could you please help check the cause of the CI error? It seems the error isn’t related to my changes. Shall we retry it?

I've restarted CI.

Please trigger the CI again, thanks.

I noticed a workflow issue and would like to get your thoughts on it.

Currently, when a new PR is merged into the dev branch, all other PRs — even those that have already passed CI — are required to update their branches and re-run CI before they can be merged.
image

This means that even PRs unrelated to the recent changes must trigger CI again, which can:

  • Increase unnecessary CI runs
  • Consume additional CI resources
  • Add extra operational overhead for contributors

I’m wondering whether we could consider a workflow similar to SeaTunnel:

  • If there is a merge conflict with the latest dev, the contributor resolves the conflict and re-runs CI.
  • If there is no conflict, re-running CI would not be required.

This might help reduce redundant CI executions and improve overall efficiency.

@SbloodyS
Copy link
Member

Currently, when a new PR is merged into the dev branch, all other PRs — even those that have already passed CI — are required to update their branches and re-run CI before they can be merged. image

This means that even PRs unrelated to the recent changes must trigger CI again, which can:

  • Increase unnecessary CI runs
  • Consume additional CI resources
  • Add extra operational overhead for contributors

I’m wondering whether we could consider a workflow similar to SeaTunnel:

  • If there is a merge conflict with the latest dev, the contributor resolves the conflict and re-runs CI.
  • If there is no conflict, re-running CI would not be required.

This might help reduce redundant CI executions and improve overall efficiency.

The consensus of PMC/Committer in this community determines that contributors do not need to pay attention to it for the time being.

@sonarqubecloud
Copy link

Copy link
Member

@SbloodyS SbloodyS left a comment

Choose a reason for hiding this comment

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

+1

Copy link
Member

@fuchanghai fuchanghai left a comment

Choose a reason for hiding this comment

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

+1

@fuchanghai fuchanghai merged commit 197667e into apache:dev Feb 28, 2026
75 checks passed
@boring-cyborg
Copy link

boring-cyborg bot commented Feb 28, 2026

Awesome work, congrats on your first merged pull request!

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

Labels

backend document first time contributor First-time contributor improvement make more easy to user or prompt friendly test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Improvement][Seatunnel] Harden startupScript validation and -i variable quoting

3 participants