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

Enable core function for archive enumeration/scanning. #2837

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

Conversation

michaelcfanning
Copy link
Member

No description provided.


if (ExtensionsAllowList != null)
{
return !ExtensionsAllowList.Contains(extension);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's surprising to me that the deny list is ignored if there is an allow list.

If users are required to use one or the other but not both, then perhaps we should check that they don't try to use both.

@@ -1,3 +1,4 @@

// Copyright (c) Microsoft. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert

@@ -253,12 +253,20 @@ public static string GetFileName(this Uri uri)
return newAbsoluteUri.AbsolutePath.Split('/').Last();
}

if (uri.Query != null)
{
return uri.Query;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would the query information be considered the file name?

}

this.uri = baseUri != null
? new Uri($"{baseUri}?{entry.FullName}")
Copy link
Contributor

Choose a reason for hiding this comment

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

?{entry.FullName}"

Oh, I see why. Yikes. Is this is a workaround for the issues in the other PR?

{
return entry.Open();
}
catch (InvalidDataException)
Copy link
Contributor

Choose a reason for hiding this comment

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

InvalidDataException

Do we not need to report this anywhere?

}
catch(InvalidDataException)
{
// TBD log corrupt zip file.
Copy link
Contributor

Choose a reason for hiding this comment

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

TBD

I guess the same TBD applies in the other catch I just commented on. Can you at least comment there too?

@nguerrera
Copy link
Contributor

I just saw that this is a draft. Silly GitHub sent me email about it. Sorry if you weren't ready for feedback.

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.

2 participants