fix: Ensure the ETag is representation specific

src/http/ldp/GetOperationHandler.ts

@@ -1,5 +1,6 @@
 import type { ResourceStore } from '../../storage/ResourceStore';
 import { NotImplementedHttpError } from '../../util/errors/NotImplementedHttpError';
+import { assertReadConditions } from '../../util/ResourceUtil';
 import { OkResponseDescription } from '../output/response/OkResponseDescription';
 import type { ResponseDescription } from '../output/response/ResponseDescription';
 import type { OperationHandlerInput } from './OperationHandler';
@@ -26,6 +27,9 @@ export class GetOperationHandler extends OperationHandler {
   public async handle({ operation }: OperationHandlerInput): Promise<ResponseDescription> {
     const body = await this.store.getRepresentation(operation.target, operation.preferences, operation.conditions);
 
+    // Check whether the cached representation is still valid or it is necessary to send a new representation
+    assertReadConditions(body, operation.conditions);
+
     return new OkResponseDescription(body.metadata, body.data);
   }
 }

src/http/ldp/HeadOperationHandler.ts

@@ -1,5 +1,6 @@
 import type { ResourceStore } from '../../storage/ResourceStore';
 import { NotImplementedHttpError } from '../../util/errors/NotImplementedHttpError';
+import { assertReadConditions } from '../../util/ResourceUtil';
 import { OkResponseDescription } from '../output/response/OkResponseDescription';
 import type { ResponseDescription } from '../output/response/ResponseDescription';
 import type { OperationHandlerInput } from './OperationHandler';
@@ -29,6 +30,10 @@ export class HeadOperationHandler extends OperationHandler {
     // Close the Readable as we will not return it.
     body.data.destroy();
 
+    // Check whether the cached representation is still valid or it is necessary to send a new representation.
+    // Generally it doesn't make much sense to use condition headers with a HEAD request, but it should be supported.
+    assertReadConditions(body, operation.conditions);
+
     return new OkResponseDescription(body.metadata);
   }
 }

src/storage/BasicConditions.ts

@@ -1,6 +1,6 @@
 import type { RepresentationMetadata } from '../http/representation/RepresentationMetadata';
 import { DC } from '../util/Vocabularies';
-import { getETag } from './Conditions';
+import { getETag, isCurrentETag } from './Conditions';
 import type { Conditions } from './Conditions';
 
 export interface BasicConditionsOptions {
@@ -26,40 +26,43 @@ export class BasicConditions implements Conditions {
     this.unmodifiedSince = options.unmodifiedSince;
   }
 
-  public matchesMetadata(metadata?: RepresentationMetadata): boolean {
+  public matchesMetadata(metadata?: RepresentationMetadata, strict?: boolean): boolean {
     if (!metadata) {
       // RFC7232: ...If-Match... If the field-value is "*", the condition is false if the origin server
       // does not have a current representation for the target resource.
       return !this.matchesETag?.includes('*');
     }
 
-    const modified = metadata.get(DC.terms.modified);
-    const modifiedDate = modified ? new Date(modified.value) : undefined;
-    const etag = getETag(metadata);
-    return this.matches(etag, modifiedDate);
-  }
-
-  public matches(eTag?: string, lastModified?: Date): boolean {
     // RFC7232: ...If-None-Match... If the field-value is "*", the condition is false if the origin server
     // has a current representation for the target resource.
     if (this.notMatchesETag?.includes('*')) {
       return false;
     }
 
-    if (eTag) {
-      if (this.matchesETag && !this.matchesETag.includes(eTag) && !this.matchesETag.includes('*')) {
-        return false;
-      }
-      if (this.notMatchesETag?.includes(eTag)) {
-        return false;
-      }
+    // Helper function to see if an ETag matches the provided metadata
+    // eslint-disable-next-line func-style
+    let eTagMatches = (tag: string): boolean => isCurrentETag(tag, metadata);
+    if (strict) {
+      const eTag = getETag(metadata);
+      eTagMatches = (tag: string): boolean => tag === eTag;
     }
 
-    if (lastModified) {
-      if (this.modifiedSince && lastModified < this.modifiedSince) {
+    if (this.matchesETag && !this.matchesETag.includes('*') && !this.matchesETag.some(eTagMatches)) {
+      return false;
+    }
+    if (this.notMatchesETag?.some(eTagMatches)) {
+      return false;
+    }
+
+    // In practice, this will only be undefined on a backend
+    // that doesn't store the modified date.
+    const modified = metadata.get(DC.terms.modified);
+    if (modified) {
+      const modifiedDate = new Date(modified.value);
+      if (this.modifiedSince && modifiedDate < this.modifiedSince) {
         return false;
       }
-      if (this.unmodifiedSince && lastModified > this.unmodifiedSince) {
+      if (this.unmodifiedSince && modifiedDate > this.unmodifiedSince) {
         return false;
       }
     }

src/storage/Conditions.ts

@@ -25,16 +25,12 @@ export interface Conditions {
   /**
    * Checks validity based on the given metadata.
    * @param metadata - Metadata of the representation. Undefined if the resource does not exist.
+   * @param strict - How to compare the ETag related headers.
+   *                 If true, exact string matching will be used to compare with the ETag for the given metadata.
+   *                 If false, it will take into account that content negotiation might still happen
+   *                 which can change the ETag.
    */
-  matchesMetadata: (metadata?: RepresentationMetadata) => boolean;
-  /**
-   * Checks validity based on the given ETag and/or date.
-   * This function assumes the resource being checked exists.
-   * If not, the `matchesMetadata` function should be used.
-   * @param eTag - Condition based on ETag.
-   * @param lastModified - Condition based on last modified date.
-   */
-  matches: (eTag?: string, lastModified?: Date) => boolean;
+  matchesMetadata: (metadata?: RepresentationMetadata, strict?: boolean) => boolean;
 }
 
 /**
@@ -45,8 +41,32 @@ export interface Conditions {
  */
 export function getETag(metadata: RepresentationMetadata): string | undefined {
   const modified = metadata.get(DC.terms.modified);
-  if (modified) {
+  const { contentType } = metadata;
+  if (modified && contentType) {
     const date = new Date(modified.value);
-    return `"${date.getTime()}"`;
+    return `"${date.getTime()}-${contentType}"`;
   }
 }
+
+/**
+ * Validates whether a given ETag corresponds to the current state of the resource,
+ * independent of the representation the ETag corresponds to.
+ * Assumes ETags are made with the {@link getETag} function.
+ * Since we base the ETag on the last modified date,
+ * we know the ETag still matches as long as that didn't change.
+ *
+ * @param eTag - ETag to validate.
+ * @param metadata - Metadata of the resource.
+ *
+ * @returns `true` if the ETag represents the current state of the resource.
+ */
+export function isCurrentETag(eTag: string, metadata: RepresentationMetadata): boolean {
+  const modified = metadata.get(DC.terms.modified);
+  if (!modified) {
+    return false;
+  }
+  const time = eTag.split('-', 1)[0];
+  const date = new Date(modified.value);
+  // `time` will still have the initial`"` of the ETag string
+  return time === `"${date.getTime()}`;
+}

src/storage/DataAccessorBasedStore.ts

@@ -7,7 +7,6 @@ import { BasicRepresentation } from '../http/representation/BasicRepresentation'
 import type { Patch } from '../http/representation/Patch';
 import type { Representation } from '../http/representation/Representation';
 import { RepresentationMetadata } from '../http/representation/RepresentationMetadata';
-import type { RepresentationPreferences } from '../http/representation/RepresentationPreferences';
 import type { ResourceIdentifier } from '../http/representation/ResourceIdentifier';
 import { getLoggerFor } from '../logging/LogUtil';
 import { INTERNAL_QUADS } from '../util/ContentTypes';
@@ -18,7 +17,6 @@ import { ForbiddenHttpError } from '../util/errors/ForbiddenHttpError';
 import { MethodNotAllowedHttpError } from '../util/errors/MethodNotAllowedHttpError';
 import { NotFoundHttpError } from '../util/errors/NotFoundHttpError';
 import { NotImplementedHttpError } from '../util/errors/NotImplementedHttpError';
-import { NotModifiedHttpError } from '../util/errors/NotModifiedHttpError';
 import { PreconditionFailedHttpError } from '../util/errors/PreconditionFailedHttpError';
 import type { IdentifierStrategy } from '../util/identifiers/IdentifierStrategy';
 import { concat } from '../util/IterableUtil';
@@ -105,8 +103,7 @@ export class DataAccessorBasedStore implements ResourceStore {
     }
   }
 
-  public async getRepresentation(identifier: ResourceIdentifier,
-    preferences?: RepresentationPreferences, conditions?: Conditions): Promise<Representation> {
+  public async getRepresentation(identifier: ResourceIdentifier): Promise<Representation> {
     this.validateIdentifier(identifier);
     let isMetadata = false;
 
@@ -119,8 +116,6 @@ export class DataAccessorBasedStore implements ResourceStore {
     let metadata = await this.accessor.getMetadata(identifier);
     let representation: Representation;
 
-    this.validateConditions(true, conditions, metadata);
-
     // Potentially add auxiliary related metadata
     // Solid, §4.3: "Clients can discover auxiliary resources associated with a subject resource by making an HTTP HEAD
     // or GET request on the target URL, and checking the HTTP Link header with the rel parameter"
@@ -186,7 +181,7 @@ export class DataAccessorBasedStore implements ResourceStore {
       throw new MethodNotAllowedHttpError([ 'POST' ], 'The given path is not a container.');
     }
 
-    this.validateConditions(false, conditions, parentMetadata);
+    this.validateConditions(conditions, parentMetadata);
 
     // Solid, §5.1: "Servers MAY allow clients to suggest the URI of a resource created through POST,
     // using the HTTP Slug header as defined in [RFC5023].
@@ -251,7 +246,7 @@ export class DataAccessorBasedStore implements ResourceStore {
       await this.accessor.canHandle(representation);
     }
 
-    this.validateConditions(false, conditions, oldMetadata);
+    this.validateConditions(conditions, oldMetadata);
 
     if (this.metadataStrategy.isAuxiliaryIdentifier(identifier)) {
       return await this.writeMetadata(identifier, representation);
@@ -273,7 +268,7 @@ export class DataAccessorBasedStore implements ResourceStore {
         }
       }
 
-      this.validateConditions(false, conditions, metadata);
+      this.validateConditions(conditions, metadata);
     }
 
     throw new NotImplementedHttpError('Patches are not supported by the default store.');
@@ -314,7 +309,7 @@ export class DataAccessorBasedStore implements ResourceStore {
       throw new ConflictHttpError('Can only delete empty containers.');
     }
 
-    this.validateConditions(false, conditions, metadata);
+    this.validateConditions(conditions, metadata);
 
     // Solid, §5.4: "When a contained resource is deleted,
     // the server MUST also delete the associated auxiliary resources"
@@ -352,13 +347,10 @@ export class DataAccessorBasedStore implements ResourceStore {
   /**
    * Verify if the given metadata matches the conditions.
    */
-  protected validateConditions(read: boolean, conditions?: Conditions, metadata?: RepresentationMetadata): void {
+  protected validateConditions(conditions?: Conditions, metadata?: RepresentationMetadata): void {
     // The 412 (Precondition Failed) status code indicates
     // that one or more conditions given in the request header fields evaluated to false when tested on the server.
     if (conditions && !conditions.matchesMetadata(metadata)) {
-      if (read) {
-        throw new NotModifiedHttpError();
-      }
       throw new PreconditionFailedHttpError();
     }
   }

src/util/ResourceUtil.ts

@@ -3,6 +3,8 @@ import { DataFactory } from 'n3';
 import { BasicRepresentation } from '../http/representation/BasicRepresentation';
 import type { Representation } from '../http/representation/Representation';
 import { RepresentationMetadata } from '../http/representation/RepresentationMetadata';
+import type { Conditions } from '../storage/Conditions';
+import { NotModifiedHttpError } from './errors/NotModifiedHttpError';
 import { guardedStreamFrom } from './StreamUtil';
 import { toLiteral } from './TermUtil';
 import { CONTENT_TYPE_TERM, DC, LDP, RDF, SOLID_META, XSD } from './Vocabularies';
@@ -65,3 +67,25 @@ export async function cloneRepresentation(representation: Representation): Promi
   representation.data = guardedStreamFrom(data);
   return result;
 }
+
+/**
+ * Verify whether the given {@link Representation} matches the given conditions.
+ * If not, destroy the data stream and throw a {@link NotModifiedHttpError}.
+ * If `conditions` is not defined, nothing will happen.
+ *
+ * This uses the strict conditions check which takes the content type into account;
+ * therefore, this should only be called after content negotiation, when it is certain what the output will be.
+ *
+ * Note that browsers only keep track of one ETag, and the Vary header has no impact on this,
+ * meaning the browser could send us the ETag for a Turtle resource even though it is requesting JSON-LD;
+ * this is why we have to check ETags after content negotiation.
+ *
+ * @param body - The representation to compare the conditions against.
+ * @param conditions - The conditions to assert.
+ */
+export function assertReadConditions(body: Representation, conditions?: Conditions): void {
+  if (conditions && !conditions.matchesMetadata(body.metadata, true)) {
+    body.data.destroy();
+    throw new NotModifiedHttpError();
+  }
+}