[App Service] Fix #30357: az webapp config ssl bind: Use full Site object to avoid Azure Policy denial#33061
Closed
[App Service] Fix #30357: az webapp config ssl bind: Use full Site object to avoid Azure Policy denial#33061
az webapp config ssl bind: Use full Site object to avoid Azure Policy denial#33061Conversation
…Site object to avoid Azure Policy denial Previously, _update_host_name_ssl_state constructed a minimal Site object with only host_name_ssl_states, location, and tags. When passed to begin_create_or_update, this caused Azure Policy (e.g. 'HTTPS Only must be enabled') to deny the operation because policy-sensitive fields like httpsOnly were missing from the payload. The fix reuses the full existing Site object fetched from Azure, updating only the host_name_ssl_states field. This preserves all policy-sensitive properties (httpsOnly, ftpsState, etc.) in the PUT request. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
seligj95
requested review from
NoriZC,
teresaritorto and
yanzhudd
as code owners
❌AzureCLI-FullTest
|
|
Hi @seligj95, |
️✔️AzureCLI-BreakingChangeTest
|
Collaborator
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
|
The git hooks are available for azure-cli and azure-cli-extensions repos. They could help you run required checks before creating the PR. Please sync the latest code with latest dev branch (for azure-cli) or main branch (for azure-cli-extensions). pip install azdev --upgrade
azdev setup -c <your azure-cli repo path> -r <your azure-cli-extensions repo path>
|
microsoft-github-policy-service
bot
requested review from
jsntcy,
wangzelin007,
yonzhan and
zhoxing-ms
microsoft-github-policy-service
bot
added
Web Apps
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes Azure Policy denial for az webapp config ssl bind by ensuring the update PUT includes policy-sensitive site fields (e.g., httpsOnly) rather than sending a minimal Site payload.
Changes:
- Update
_update_host_name_ssl_stateto reuse the existingSiteobject instead of constructing a minimal one. - Add unit tests validating the full
Siteobject is passed through and that theslotargument is forwarded.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/azure-cli/azure/cli/command_modules/appservice/custom.py |
Reuses the retrieved Site object when calling begin_create_or_update for SSL binding updates. |
src/azure-cli/azure/cli/command_modules/appservice/tests/latest/test_webapp_commands_thru_mock.py |
Adds tests around _update_host_name_ssl_state behavior (full site passthrough + slot forwarding). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+6045
to
+6051
| HostNameSslState = cmd.get_models('HostNameSslState') | ||
| webapp.host_name_ssl_states = [HostNameSslState(name=host_name, | ||
| ssl_state=ssl_state, | ||
| thumbprint=thumbprint, | ||
| to_update=True)] | ||
| return _generic_site_operation(cmd.cli_ctx, resource_group_name, webapp_name, 'begin_create_or_update', | ||
| slot, updated_webapp) | ||
| slot, webapp) |
There was a problem hiding this comment.
When slot is provided, this helper assumes the passed webapp object represents that slot. In the current call path (_update_ssl_binding fetches client.web_apps.get(...) regardless of slot), this can cause a slot PUT (begin_create_or_update_slot) to reuse the production Site payload (including policy-sensitive fields like https_only), potentially triggering policy denial or unintentionally overwriting slot-specific settings. Consider fetching the Site for the slot (client.web_apps.get_slot(...) or _generic_site_operation(..., 'get', slot)) and using that object for the update when slot is not None.
Comment on lines
+692
to
+693
| # site is the 6th positional arg (index 5) | ||
| self.assertIs(call_args[0][5], webapp) |
There was a problem hiding this comment.
test_update_host_name_ssl_state_with_slot asserts object identity (assertIs(..., webapp)), which will fail if the implementation is adjusted to avoid in-place mutation or to refetch the correct slot Site (which is likely needed for correctness). It would be less brittle to assert on the payload’s relevant properties (e.g., https_only, location, tags) and/or add a focused test at the _update_ssl_binding level to ensure slot updates use get_slot-retrieved site data.
Suggested change
| # site is the 6th positional arg (index 5) | |
| self.assertIs(call_args[0][5], webapp) | |
| # site is the 6th positional arg (index 5); verify it has the expected properties | |
| site_arg = call_args[0][5] | |
| self.assertEqual(site_arg.name, webapp.name) | |
| self.assertEqual(site_arg.location, webapp.location) |
- In _update_ssl_binding, use get_slot() when slot is provided to avoid sending production Site payload for slot updates (policy/settings issue) - Replace assertIs with property assertions in slot test for robustness Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
|
Consolidated into #33058 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #30357
az webapp config ssl bindwas constructing a minimalSiteobject with onlyhost_name_ssl_states,location, andtagswhen callingbegin_create_or_update. This caused Azure Policy (e.g. "HTTPS Only must be enabled") to deny the operation because policy-sensitive fields likehttpsOnlywere absent from the PUT payload.Changes
custom.py: Modified_update_host_name_ssl_stateto reuse the full existingSiteobject (already fetched by the caller) instead of constructing a minimal one. Only thehost_name_ssl_statesfield is updated on the existing object before passing it tobegin_create_or_update.test_webapp_commands_thru_mock.py: AddedTestUpdateHostNameSslStatewith two tests:https_only,location,tags) is passed throughslotparameter is correctly forwardedTesting
test_webapp_commands_thru_mock.pypass