Skip to content

Conversation

galeksandrp
Copy link

@galeksandrp galeksandrp commented Feb 16, 2019

Request headers support in Get-WebHeaders is required for fixing bug in Get-ChocolateyWebFile.

When no checksum is passed to Get-ChocolateyWebFile, it's check file length: response headers is fetched by Get-WebHeaders using HEAD request and then compared with file length which got from file fetched with GET request.

File length check should succeed, because HEAD request to equal URL, using same request headers as GET request, will return same Content-Length.

Actually file length check will not succeed, because Get-WebHeaders does not support request headers except User-Agent, and therefore headers accepted by Get-ChocolateyWebFile is not passed to Get-WebHeaders.

Monkey-patched version of Get-ChocolateyWebFile is used by AU for generating checksums on-the-fly. This PR is part of effort introducing support of request headers in AU.

Request headers passed as -Options @{Headers=@{}}, not as just -Headers @{}, for mantaining consistency with others Chocolatey's helper functions, such as Get-ChocolateyWebFile.

Request headers support in `Get-WebHeaders` is required for fixing bug
in `Get-ChocolateyWebFile`.

When no checksum is passed to `Get-ChocolateyWebFile`, it's check file
length: response headers is fetched by `Get-WebHeaders` using `HEAD`
request and then compared with file length which got from file fetched
with `GET` request.

File length check should succeed, because `HEAD` request to equal URL,
using same request headers as `GET` request, will return same
`Content-Length`.

Actually file length check will not succeed, because `Get-WebHeaders`
does not support request headers except `User-Agent`, and therefore
headers accepted by `Get-ChocolateyWebFile` is not passed to
`Get-WebHeaders`.

Monkey-patched version of `Get-ChocolateyWebFile` is used by
[`AU`](https://github.com/majkinetor/au) for generating checksums
on-the-fly. This PR is part of effort introducing support of request
headers in `AU`.

Request headers passed as `-Options @{Headers=@{}}`, not as just
`-Headers @{}`, for mantaining consistency with others `Chocolatey`'s
helper functions, such as `Get-ChocolateyWebFile`.
Copy link
Member

@ferventcoder ferventcoder left a comment

Choose a reason for hiding this comment

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

Howdy! Thanks for this. Love that the commit message had so much information, that was helpful!

There are a couple of small changes that need to be addressed here, but overall great work!

[parameter(ValueFromRemainingArguments = $true)][Object[]] $ignoredArguments
[parameter(Mandatory=$false, Position=1)][string] $userAgent,
[parameter(ValueFromRemainingArguments = $true)][Object[]] $ignoredArguments,
[parameter(Mandatory=$false)][hashtable] $options = @{Headers=@{}}
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be before $ignoredArguments. Those are the last items to be passed.

if ($userAgent -ne $null) {

#Mimic previous behaviour with default User-Agent
$request.UserAgent = 'chocolatey command line'
Copy link
Member

Choose a reason for hiding this comment

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

Formatting please

.PARAMETER Options
Specify request headers. Please note that UserAgent function parameter, if specified, overwrite header present in this parameter.
Copy link
Member

Choose a reason for hiding this comment

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

This should also specify the first version this was available in. I would venture a guess that it is 0.10.13. At least include that here (search other parameters for examples of this and keep it consistent.). Thanks!

@ferventcoder
Copy link
Member

Oh and one more thing - this needs an issue created (if there is not one), and then the commit message needs to refer to that - see https://github.com/chocolatey/choco/blob/master/CONTRIBUTING.md for more information.

@ferventcoder
Copy link
Member

@ferventcoder
Copy link
Member

@galeksandrp are you in a place to make changes?

@grahamgilbert
Copy link

We are also experiencing this issue. What is holding up getting this merged?

@CLAassistant
Copy link

CLAassistant commented Jun 8, 2020

CLA assistant check
All committers have signed the CLA.

@pauby pauby mentioned this pull request May 12, 2025
10 tasks
@vexx32
Copy link
Member

vexx32 commented May 15, 2025

Thanks for your contribution here! This work is encompassed and added to by the work done in #3395, so I'll be closing this one for now.

@vexx32 vexx32 closed this May 15, 2025
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.

5 participants