Skip to content

Fix #49169: SoapServer calls wrong function, although "SOAP action" header is correct - #15970

Closed
ndossche wants to merge 3 commits into
php:masterfrom
ndossche:fix-49169
Closed

ndossche wants to merge 3 commits into
php:masterfrom
ndossche:fix-49169

Conversation

@ndossche

Copy link
Copy Markdown
Member

See individual commits and their descriptions.
Sending to master because the fix is fairly non-trivial.

Comment thread ext/soap/soap.c Outdated

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

As stated by the TODO, this may require more work but depends on another fix.

@ndossche

Copy link
Copy Markdown
Member Author

@ndossche

ndossche commented Sep 20, 2024 •

Copy link
Copy Markdown
Member Author

cc @cmb69 You seem to have some SOAP knowledge, perhaps you are interested in sanity checking this?
EDIT: I'll try to figure out the failure, I didn't see this yet when I submitted the PR

@ndossche
ndossche force-pushed the fix-49169 branch 3 times, most recently from 48ff6e7 to 7fe2c05 Compare September 21, 2024 17:15
@ndossche

Copy link
Copy Markdown
Member Author

The failure was because of 2 missing NULL checks, one for the SDL, and one for the binding attributes. Remaining failures are unrelated.

@Girgias Girgias left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Overall this looks sensible, but maybe wait for master to be 8.5 rather than 8.4?

@cmb69

cmb69 commented Sep 22, 2024

Copy link
Copy Markdown
Member

You seem to have some SOAP knowledge, […]

Not really. :(

[…] but maybe wait for master to be 8.5 rather than 8.4?

Given that this is a bug fix, I think 8.4 is fine. On the other hand, the bug has been reported 15 years ago, so having to wait another year for the fix might be bearable.

@ndossche

Copy link
Copy Markdown
Member Author

I'm okay with waiting for 8.5 to put through a bunch of fixes into SOAP.

This is in preparation for adding functionality in later commits.
…eader is correct

Although the original reproducer no longer exists, I was able to cook up
something similar.
The problem is that there are two ways ext-soap currently looks up
functions:
1) By matching the exact function name; but this doesn't work if the
   function name is not in the body.
2) By matching the parameter names.

Neither of these work when we don't have the function name in the body,
and when the parameter names are not unique. That's where we can use the
"SOAPAction" header to distinguish between different actions. This header
should be checked first and be matched against the "soapAction"
attribute in the WSDL. We keep the existing fallbacks such that the
chance of a BC break is minimized.
Note that since #49169 a potential target namespace is ignored right
now.
@ndossche

Copy link
Copy Markdown
Member Author

Rebased on current master so entire CI can test.

@ndossche

Copy link
Copy Markdown
Member Author

If no one objects I'll merge on monday

@ndossche ndossche closed this in 63e0b9c Sep 30, 2024
jorgsowa pushed a commit to jorgsowa/php-src that referenced this pull request Oct 1, 2024
…eader is correct

Although the original reproducer no longer exists, I was able to cook up
something similar.
The problem is that there are two ways ext-soap currently looks up
functions:
1) By matching the exact function name; but this doesn't work if the
   function name is not in the body.
2) By matching the parameter names.

Neither of these work when we don't have the function name in the body,
and when the parameter names are not unique. That's where we can use the
"SOAPAction" header to distinguish between different actions. This header
should be checked first and be matched against the "soapAction"
attribute in the WSDL. We keep the existing fallbacks such that the
chance of a BC break is minimized.
Note that since #49169 a potential target namespace is ignored right
now.

Closes phpGH-15970.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants