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

488 prevent nested subdomain #674

Merged
merged 6 commits into from
Aug 18, 2022
Merged

488 prevent nested subdomain #674

merged 6 commits into from
Aug 18, 2022

Conversation

devinsag
Copy link
Contributor

@devinsag devinsag requested a review from tjhiggins August 17, 2022 19:06
@@ -34,6 +34,10 @@ export class Slugs {
public static ComponentSecretDescription = 'must contain alphanumeric character ([a-z0-9A-Z]), could contain dashes (-), underscores (_), and alphanumerics between.';
public static ComponentSecretRegexBase = `[a-zA-Z0-9_-]+`;
public static ComponentSecretValidator = new RegExp(`^${Slugs.ComponentSecretRegexBase}$`);

public static ComponentSubdomainRegexBase = '([A-Za-z0-9](?:[A-Za-z0-9\\-]{0,61}[A-Za-z0-9])|[\\*|\\@])?';
public static ComponentSubdomainDescription = 'must contain alphanumeric character ([a-z0-9A-Z]), could contain dashes (-), underscores (_), and alphanumerics between.';
Copy link
Member

Choose a reason for hiding this comment

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

🍝 don't just copy and paste. The above regex doesn't support underscores.

regexr.com/6s3j5

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! 'll modify the description to remove the underscore part since it's not part of the regex expression.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

modified description to remove underscore as a valid character

'DDD',
'DDD-DDD',
'DD-DD-DD-DD',
'D-----D',
Copy link
Member

Choose a reason for hiding this comment

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

Could you pick a different letter than D? :P

Copy link
Contributor Author

@devinsag devinsag Aug 17, 2022

Choose a reason for hiding this comment

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

Added a variety of characters and numbers to the test cases. additionally added more invalid characters to the regex that could slip through if one tried to pass |

@@ -39,8 +39,9 @@ export class IngressSpec {

@IsOptional()
@JSONSchema({
type: 'string',
...ExpressionOr({ type: 'string', pattern: Slugs.ComponentSubdomainValidator.source }),
Copy link
Member

Choose a reason for hiding this comment

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

Did you test deploying with a subdomain set via a secret?

This probably needs a test interfaces.test.ts to check the ingresses interpolation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially I had the spec added without ExpressionOr, but component_config2 in interpolation.test.ts would trigger validation errors. once I added the expressionOr, it started passing valid expressions again. I'll add a test to make sure adding an invalid subdomain by interpolation is still blocked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added tests to interfaces to test both valid and invalid subdomain values passed through secrets

Copy link
Member

Choose a reason for hiding this comment

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

Did you actually run architect dev to test?

@devinsag devinsag requested a review from tjhiggins August 17, 2022 23:04
@tjhiggins tjhiggins merged commit 0445430 into rc Aug 18, 2022
@tjhiggins tjhiggins deleted the 488-prevent-nested-subdomain branch August 18, 2022 13:38
github-actions bot pushed a commit that referenced this pull request Aug 18, 2022
# [1.24.0-rc.3](v1.24.0-rc.2...v1.24.0-rc.3) (2022-08-18)

### Bug Fixes

* **subdomain:** 488 prevent nested subdomain ([#674](#674)) ([0445430](0445430))
github-actions bot pushed a commit that referenced this pull request Sep 1, 2022
# [1.25.0-rc.1](v1.24.0...v1.25.0-rc.1) (2022-09-01)

### Bug Fixes

* **cli:** convert deployCommand auto-approve ([#685](#685)) ([78f3209](78f3209))
* **cli:** Docker verify improved ([#679](#679)) ([552cd77](552cd77))
* **dev:** Dev command leaves containers running when process exits with an error ([#677](#677)) ([b8e5165](b8e5165))
* **dev:** Fixed host regex that shouldnt have a global match that caused regexp.exec to not work as desired ([#689](#689)) ([1a6428d](1a6428d))
* **dev:** fixing healthcheck liveness probe protocol for port/path ([#678](#678)) ([81e69f0](81e69f0))
* **dev:** Handle edge cases when starting components with pre-existing containers ([9cffb28](9cffb28))
* **exec:** Handle case where older version of compose is used and ConfigFiles doesnt exist ([#681](#681)) ([c0112d2](c0112d2))
* **exec:** Handle terminal resize events in exec ([#675](#675)) ([6ff95a5](6ff95a5))
* **exec:** Improve error message for Windows PS users ([#680](#680)) ([c214870](c214870))
* **subdomain:** 488 prevent nested subdomain ([#674](#674)) ([0445430](0445430))

### Features

* **cli:** 496 consistent boolean flags ([#683](#683)) ([4203c74](4203c74))
* **dev:** Use local SSL for  ([#676](#676)) ([536b38e](536b38e))
* **exec:** 482 pass replica name ([#663](#663)) ([84ce8c5](84ce8c5))
* **register:** add register multiple components and test cases ([#657](#657)) ([2bd1fd2](2bd1fd2))
github-actions bot pushed a commit that referenced this pull request Sep 1, 2022
# [1.25.0](v1.24.0...v1.25.0) (2022-09-01)

### Bug Fixes

* **cli:** convert deployCommand auto-approve ([#685](#685)) ([78f3209](78f3209))
* **cli:** Docker verify improved ([#679](#679)) ([552cd77](552cd77))
* **dev:** Dev command leaves containers running when process exits with an error ([#677](#677)) ([b8e5165](b8e5165))
* **dev:** Fixed host regex that shouldnt have a global match that caused regexp.exec to not work as desired ([#689](#689)) ([1a6428d](1a6428d))
* **dev:** fixing healthcheck liveness probe protocol for port/path ([#678](#678)) ([81e69f0](81e69f0))
* **dev:** Handle edge cases when starting components with pre-existing containers ([9cffb28](9cffb28))
* **exec:** Handle case where older version of compose is used and ConfigFiles doesnt exist ([#681](#681)) ([c0112d2](c0112d2))
* **exec:** Handle terminal resize events in exec ([#675](#675)) ([6ff95a5](6ff95a5))
* **exec:** Improve error message for Windows PS users ([#680](#680)) ([c214870](c214870))
* **subdomain:** 488 prevent nested subdomain ([#674](#674)) ([0445430](0445430))

### Features

* **cli:** 496 consistent boolean flags ([#683](#683)) ([4203c74](4203c74))
* **dev:** Use local SSL for  ([#676](#676)) ([536b38e](536b38e))
* **exec:** 482 pass replica name ([#663](#663)) ([84ce8c5](84ce8c5))
* **register:** add register multiple components and test cases ([#657](#657)) ([2bd1fd2](2bd1fd2))
@github-actions
Copy link

github-actions bot commented Sep 1, 2022

🎉 This PR is included in version 1.25.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

2 participants