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

v0.6.0 Brings support for relative URLs and paths. #73 #83

Merged
merged 13 commits into from
Feb 22, 2023
Merged

Conversation

daveshanley
Copy link
Member

@daveshanley daveshanley commented Feb 18, 2023

v0.6.0 Addresses issue #73 by introducing some enhancements to the index, allowing for relative file handling, and providing a fix to auto-allowing remote and local files to be followed.

A new configuration option is available to document creation for an Index. It's defined as index.SpecIndexConfig and provides three properties:

  • BaseURL of type *url.URL
  • AllowRemoteLookup of type bool
  • AllowFileLookup of type bool

This can be used to configure an index to know where to look when encountering relative paths and if to allow them at all.

Full documentation can be found here.

When creating a new Document, there is also a new datamodel.DocumentConfigutation, which looks almost identical, except
for the Lookup postfix has been replaced with References

config := datamodel.DocumentConfiguration{
		AllowFileReferences:   true,
		AllowRemoteReferences: true,
		BaseURL:               baseURL,
	}

This new config can be used with a new function called NewDocumentWithConfiguration() that is the same as NewDocument() except it has a second argument for accepting the config.

doc, err := NewDocumentWithConfiguration(digitalOcean, &config)

Full documentation can be found here.

The index has been refactored a little to help make it easier to navigate and tuned up to run faster when indexing references.

The digital ocean spec has been added for testing purposes.

Quite a bit of surgery required.
Tests all passing, runs super fast, pulls in every single DigitalOcean spec and parses it. There may be some issues deeper down in the models, but for now high level tests all pass.
@codecov-commenter
Copy link

codecov-commenter commented Feb 18, 2023

Codecov Report

Base: 99.72% // Head: 99.70% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (7c65074) compared to base (2d536c8).
Patch coverage: 98.79% of modified lines in pull request are covered.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #83      +/-   ##
==========================================
- Coverage   99.72%   99.70%   -0.02%     
==========================================
  Files         136      142       +6     
  Lines        7964     8136     +172     
==========================================
+ Hits         7942     8112     +170     
- Misses         22       24       +2     
Flag Coverage Δ
unittests 99.70% <98.79%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
datamodel/low/extraction_functions.go 99.65% <66.66%> (-0.35%) ⬇️
datamodel/low/v2/swagger.go 98.71% <66.66%> (-1.29%) ⬇️
document.go 88.15% <88.23%> (+2.00%) ⬆️
datamodel/low/v3/create_document.go 98.95% <88.88%> (-1.05%) ⬇️
resolver/resolver.go 98.26% <98.00%> (-0.41%) ⬇️
index/extract_refs.go 99.36% <99.36%> (ø)
index/utility_methods.go 99.40% <99.40%> (ø)
datamodel/low/reference.go 96.22% <100.00%> (-3.78%) ⬇️
datamodel/low/v3/path_item.go 100.00% <100.00%> (ø)
index/find_component.go 100.00% <100.00%> (ø)
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@daveshanley daveshanley changed the title WIP: v0.6.0 Brings support for relative URLs and paths. #73 v0.6.0 Brings support for relative URLs and paths. #73 Feb 18, 2023
I am getting tired now, time for a break.
Copy link

@n0rig n0rig left a comment

Choose a reason for hiding this comment

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

take or leave it; most of these comments are nits. I think some would be nice to address.

Otherwise, the only other comment is that maybe the index code could be brought in as another PR.

datamodel/document_config.go Show resolved Hide resolved
datamodel/high/v3/document_test.go Outdated Show resolved Hide resolved
datamodel/low/v2/swagger.go Show resolved Hide resolved
datamodel/low/v2/swagger.go Show resolved Hide resolved
datamodel/low/v3/path_item.go Show resolved Hide resolved
thing := index.FindComponentInRoot("#/valid-but-missing")
assert.Nil(t, thing)
assert.Len(t, index.GetReferenceIndexErrors(), 1)
}
Copy link

Choose a reason for hiding this comment

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

Can you add a test case for path traversal attacks.
e.g.

  • https://pb33f.io/../../../etc/passwd

Copy link
Member Author

Choose a reason for hiding this comment

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

How? without a denylist, what would the test be?

index/index_model.go Show resolved Hide resolved
index/index_utils.go Show resolved Hide resolved
resolver/resolver.go Outdated Show resolved Hide resolved
resolver/resolver.go Show resolved Hide resolved
Fixed digital ocean test fix, bumped comments up a little
tests all passing locally, lets see what the issue is.
This partially resolves a whacky path ref lookup in the index mentioned in #84, but it's not a full fix, that requires the build out of a resolved spec. The design needs thought and care.
The index can now accept multiple parameters with the same name, as long as they have different `in` types.
@daveshanley daveshanley merged commit bc1d8c5 into main Feb 22, 2023
daveshanley added a commit that referenced this pull request Feb 22, 2023
Fixed digital ocean test fix, bumped comments up a little
@daveshanley daveshanley deleted the fix-73 branch June 19, 2023 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants