NFC-102 Web eID for mobile support by SanderKondratjevNortal · Pull Request #46 · web-eid/web-eid-authtoken-validation-php

SanderKondratjevNortal · 2025-12-17T13:37:42Z

Signed-off-by: Sander Kondratjev sander.kondratjev@nortal.com

mrts · 2026-02-17T15:32:01Z

example/src/AuthContext.php

+            $base64ChallengeNonce
+        );
+
+        session_regenerate_id();


Session is regenerated twice, see Auth.validate() line 80.

mrts · 2026-02-17T15:33:55Z

example/src/AuthContext.php

+        session_regenerate_id();
+
+        $subjectName = $this->getPrincipalNameFromCertificate($cert);
+        $_SESSION["auth-user"] = $subjectName;


I would suggest to move all session-related logic either up into Auth.validate() (along with session regeneration) or here for consistency.

mrts · 2026-02-17T15:40:39Z

example/src/MobileAuth.php

+        if (str_starts_with($baseUrl, 'http')) {
+            $authUri = rtrim($baseUrl, '/') . '/auth#' . $encodedPayload;
+        } else {
+            $authUri = rtrim($baseUrl, '/') . '//auth#' . $encodedPayload;
+        }


Maybe ternary conditional would be more clear:

Suggested change

if (str_starts_with($baseUrl, 'http')) {

$authUri = rtrim($baseUrl, '/') . '/auth#' . $encodedPayload;

} else {

$authUri = rtrim($baseUrl, '/') . '//auth#' . $encodedPayload;

}

$fragment = (str_starts_with($baseUrl, 'http') ? '//' : '/') . 'auth#';

$authUri = rtrim($baseUrl, '/') . $fragment . $encodedPayload;

mrts · 2026-02-17T15:45:15Z

example/src/AuthContext.php

+
+    public function assertCsrf(bool $jsonError = true): void
+    {
+        $headers = array_change_key_case(getallheaders());


Explicit is better than implicit:

Suggested change

$headers = array_change_key_case(getallheaders());

$headers = array_change_key_case(getallheaders(), CASE_LOWER);

mrts · 2026-04-07T13:27:58Z

example/src/AuthContext.php

+        return (new AuthTokenValidatorBuilder($logger))
+            ->withSiteOrigin(new Uri($this->config->get('origin_url')))
+            ->withTrustedCertificateAuthorities(...$this->trustedIntermediateCACertificates())
+            ->withOcspRequestTimeout(5)


Is explicit OCSP timeout required here, can we omit this and rely on the default instead?

Yes, we can, removed. Default: src/validator/AuthTokenValidatorBuilder.php:125

Also used prettier

mrts · 2026-04-07T13:32:57Z

example/tpl/index.phtml

+                      <code
+                      >~/Library/Containers/eu.web-eid.web-eid/Data/Library/Application\
+                        Support/RIA/web-eid/web-eid.log</code
+                      >


Formatting issue, remove line breaks from HTML tags:

Suggested change

<code

>~/Library/Containers/eu.web-eid.web-eid/Data/Library/Application\

Support/RIA/web-eid/web-eid.log</code

>

<code>

~/Library/Containers/eu.web-eid.web-eid/Data/Library/Application\

Support/RIA/web-eid/web-eid.log

</code>

Reformat with a HTML formatting tool for consistent formatting.

mrts · 2026-04-07T13:33:21Z

example/tpl/index.phtml

+                      in macOS
+                    </li>
+                    <li>
+                      <code


Formatting issue here as well, tags have line breaks inside.

mrts · 2026-04-07T13:33:26Z

example/tpl/index.phtml

+                      of Safari in macOS
+                    </li>
+                    <li>
+                      <code


Formatting issue here as well, tags have line breaks inside.

mrts · 2026-04-07T13:33:46Z

example/tpl/index.phtml

+              <p>
+                Technical overview of the solution is available in the project
+                <a href="https://github.com/web-eid/web-eid-system-architecture-doc"
+                >system architecture document</a


Formatting issue here as well, tags have line breaks inside.

mrts · 2026-04-07T14:02:55Z

src/certificate/CertificateLoader.php

+        } catch (Throwable) {
+            throw new CertificateDecodingException("'{$fieldName}' decode failed");


Generally the recommendation is that lower layers like current library code should catch Exceptions, not Throwables. Throwables should be caught at the outer application layer.

Also, would it help to include the original exception?

Suggested change

} catch (Throwable) {

throw new CertificateDecodingException("'{$fieldName}' decode failed");

} catch (Throwable $e) {

throw new CertificateDecodingException($e, "'{$fieldName}' decode failed");

Also used prettier

mrts · 2026-04-07T14:05:14Z

src/validator/certvalidators/SubjectCertificateNotRevokedValidator.php

            }
+        } catch (UserCertificateOCSPCheckFailedException $e) {
+                throw $e;
        } catch (Throwable $e) {


Same Throwable problem in existing code.

mrts · 2026-04-07T14:06:43Z

src/validator/certvalidators/SubjectCertificateNotRevokedValidator.php

    }

    private static function checkNonce(OcspRequest $request, OcspBasicResponse $basicResponse): void
-    {


Broken indents, reformat with PHP formatting tool.

Used prettier

mrts · 2026-04-07T14:13:09Z

src/validator/versionvalidators/AuthTokenVersion11Validator.php

+
+        if ($signingCertificates === null || empty($signingCertificates)) {
+            throw new AuthTokenParseException(
+                "'unverifiedSigningCertificates' field is missing, null or empty for format 'web-eid:1.1'"


Can unverifiedSigningCertificates legitimately be empty, for example when the signing certificate does not exist on the eID token or the reader cannot read the signing cert etc? How did you specify this? (This applies to .NET and Java, too.)

In the current web-eid:1.1, unverifiedSigningCertificates is expected to be present and contain at least one entry. The PHP, Java and .NET validators all enforce that intentionally.
This is based on the current token format definition (will be also in the readme).

mrts · 2026-04-07T14:17:13Z

src/validator/versionvalidators/AuthTokenVersion1Validator.php

+
+        try {
+            $loaded = $subjectCertificate->loadX509($authToken->getUnverifiedCertificate());
+            } catch (Throwable $e) {


Another Throwable.

Also used prettier

…ort for web-eid example Signed-off-by: Sander Kondratjev <sander.kondratjev@nortal.com>

Signed-off-by: Sander Kondratjev <sander.kondratjev@nortal.com>

…bile auth error layout Signed-off-by: Sander Kondratjev <sander.kondratjev@nortal.com>

Signed-off-by: Sander Kondratjev <sander.kondratjev@nortal.com>

SanderKondratjevNortal force-pushed the NFC-102 branch 3 times, most recently from 363add3 to 1d5a80e Compare January 26, 2026 09:08

SanderKondratjevNortal changed the base branch from main to web-eid-mobile January 26, 2026 09:10

mrts requested changes Feb 17, 2026

View reviewed changes

SanderKondratjevNortal force-pushed the NFC-102 branch from 4f22693 to a21b41c Compare March 24, 2026 12:16

mrts requested changes Apr 7, 2026

View reviewed changes

SanderKondratjevNortal added 6 commits April 8, 2026 14:51

NFC-102 Add web-eid-1.1 token support with Mobile authentication supp…

0608e57

…ort for web-eid example Signed-off-by: Sander Kondratjev <sander.kondratjev@nortal.com>

NFC-102 Review findings

5df5711

Signed-off-by: Sander Kondratjev <sander.kondratjev@nortal.com>

NFC-127 Upgrade PHP version to 8.5

f78885e

Signed-off-by: Sander Kondratjev <sander.kondratjev@nortal.com>

NFC-118 Switch request/response JSON fields to camelCase and unify mo…

49098ab

…bile auth error layout Signed-off-by: Sander Kondratjev <sander.kondratjev@nortal.com>

NFC-115 Implement minimal multiple signing certification solution

d8fbe65

Signed-off-by: Sander Kondratjev <sander.kondratjev@nortal.com>

NFC-135 Update EU funding logo

871c0fa

Signed-off-by: Sander Kondratjev <sander.kondratjev@nortal.com>

SanderKondratjevNortal force-pushed the NFC-102 branch from a21b41c to 871c0fa Compare April 8, 2026 11:52

NFC-102 Review findings

1e0cd51

Signed-off-by: Sander Kondratjev <sander.kondratjev@nortal.com>

	$headers = array_change_key_case(getallheaders());
	$headers = array_change_key_case(getallheaders(), CASE_LOWER);

		} catch (Throwable) {
		throw new CertificateDecodingException("'{$fieldName}' decode failed");

Conversation

SanderKondratjevNortal commented Dec 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants