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

Python/JavaScript: Shared module for serverless functions #13729

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

Conversation

yoff
Copy link
Contributor

@yoff yoff commented Jul 12, 2023

With this, we find 2 command injections in DVSA.

@yoff yoff force-pushed the python/model-aws-lambdas branch from 3d9d685 to dc0282d Compare July 12, 2023 12:30
@github-actions github-actions bot added the JS label Jul 12, 2023
yoff added 3 commits July 12, 2023 16:42
made space for other serverless frameworks in the directory `serverless`
Modelled on the javascript serverless module, but
- The predicate that reports YAML files is now public
  so languages can implement their own file conventions.
- It also reports framework and runtime.
- The conveninece predicates with files still exist,
  but they only report the path.
- Handler mapping conventions are now documented.
- Use parameterised serverless module in Python,
  tests now pass.
@yoff yoff force-pushed the python/model-aws-lambdas branch from a867c37 to 02c41f3 Compare July 12, 2023 14:48
@yoff yoff added the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Jul 12, 2023
Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>
@yoff yoff marked this pull request as ready for review July 17, 2023 16:42
@yoff yoff requested review from a team as code owners July 17, 2023 16:42
@alexrford alexrford self-requested a review July 19, 2023 13:50
alexrford
alexrford previously approved these changes Jul 23, 2023
Copy link
Contributor

@alexrford alexrford left a comment

Choose a reason for hiding this comment

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

Just some very minor nitpicks, looks very good.

Definitely also looks like something we should consider porting for Ruby - at first glance it doesn't seem like it would take an enormous amount of work to do this.

shared/yaml/codeql/serverless/mapping.md Outdated Show resolved Hide resolved
shared/yaml/codeql/serverless/mapping.md Outdated Show resolved Hide resolved
shared/yaml/codeql/serverless/mapping.md Outdated Show resolved Hide resolved
shared/yaml/codeql/serverless/ServerLess.qll Outdated Show resolved Hide resolved
* Gets a string suitable as part of a file path.
*/
bindingset[base]
private string normalise(string base) { result = removeLeadingDotSlash(removeTrailingDot(base)) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be US spelling normalize. Also maybe a more descriptive name, e.g. normalizePath?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that is a better name :-)

Comment on lines 103 to 105
if exists(properties.lookup("CodeUri"))
then codeUri = normalise(lookupValue(properties, "CodeUri"))
else codeUri = ""
Copy link
Contributor

Choose a reason for hiding this comment

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

This "if property exists then lookup the value, else """ pops up a few times in this predicate (also on L108 and L123) - maybe worth factoring out into a little helper predicate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

Co-authored-by: Alex Ford <alexrford@users.noreply.github.com>
- rename `normalise` to `normalizePath`
- factor out `lookupValueOrEmpty`
Co-authored-by: Alex Ford <alexrford@users.noreply.github.com>
Copy link
Contributor

@alexrford alexrford left a comment

Choose a reason for hiding this comment

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

LGTM - there's one QLDoc failure for Python ServerLess.qll and this is worth adding a changenote for. Otherwise I'm happy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish documentation JS Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants