Parse malformed clocklines (was: Re: [PATCH] lisp/org-clock.el (org-clock-sum): Rewrite regex using rx)

[Morgan Smith]

Ihor Radchenko  writes:

> Morgan Smith  writes:
>
>>> That's expected.
>>> We have the following _syntax_ description for clock lines:
>>>
>>> https://orgmode.org/worg/org-syntax.html#Clocks...>> clock: 
>>> INACTIVE-TIMESTAMP-RANGE DURATION
>> ...
>> My specific issue is that the ":*-end" stuff can be set when the
>> "*-start" stuff is not.
>> ...
>> CLOCK: [2023-11-15 Wed 15rr:26]--[2023-11-15 Wed 16:12] =>  0:46
>> calculated time: 58320.0
>> ...
>> What this shows is that an invalid end will cause the entry to be
>> ignored, but an invalid start will not.
>
> Yup. Everything makes sense, but only within syntax spec. Actual code
> that handles such clock lines is another story.
>
> Warning is important because a number of people use clock functionality
> for billing - miscalculating clock sums can literally affect people's
> income :)
>
> I think that the best course of action when a problematic timestamp
> without opening/closing time is encountered is:
>
> 1. Warn user
> 2. Still calculate the duration, assuming 0s in time (simply because
>previous versions of Org mode did it)
>
> (2) is kind of debatable, but I can imagine some users might make use of
>
> such feature by putting clocks by hand:
> CLOCK: [2023-11-15 Wed]--[2023-11-16 Thu] => 24:00
>
> These users may then simply suppress the warning.

I'm very tempted to just make it hard rule that clocklines need to be
fully specified.  If you take a look at the attached patch, it shows one
way we could do this.

First, I modify the timestamp parser so the ":*-end" variables don't
always inherit the ":*-start" variables.  They can be nil independent of
the start.

Then in the clockline parser I ensure that every single field is set.
If it isn't, then I set the timestamp to nil so it won't be used.

This preserves the flexibility you want in timestamp parsing while also
catching many malformed clocklines.

Let me know what you think of this approach.

Also a couple questions about this approach:

1. Will changing the timestamp parser have far-reaching implications?
Is this something I should avoid?

2. Is there a way to give user errors in the parser code?  I'm using
`org-element--cache-warn' in my patch but I'm not sure that's the right
thing to do.  (if it is the right thing then we need to define it
earlier in the file)


>From 82231fb4b11b780488c66b4e5f0ee6fcf6643f1d Mon Sep 17 00:00:00 2001
From: Morgan Smith 
Date: Wed, 19 Jun 2024 13:41:50 -0400
Subject: [PATCH] Warn about invalid clock lines

---
 lisp/org-element.el | 32 +---
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/lisp/org-element.el b/lisp/org-element.el
index d6ad9824a..113cd6059 100644
--- a/lisp/org-element.el
+++ b/lisp/org-element.el
@@ -2312,6 +2312,25 @@ Return a new syntax node of `clock' type containing `:status',
 			 (unless (bolp) (skip-chars-forward " \t"))
 			 (count-lines before-blank (point
 	   (end (point)))
+  (when (and value ;; Clock lines don't need timestamps
+ (or
+  (not
+   (and (org-element-property :year-start value)
+(org-element-property :month-start value)
+(org-element-property :day-start value)
+(org-element-property :hour-start value)
+(org-element-property :minute-start value)))
+  (and (eq status 'closed)
+   (not (and (org-element-property :year-end value)
+ (org-element-property :month-end value)
+ (org-element-property :day-end value)
+ (org-element-property :hour-end value)
+ (org-element-property :minute-end value))
+(setq value nil)
+(org-element--cache-warn "Invalid clock element at %s:%d: \"%s\""
+ (buffer-name)
+ (line-number-at-pos begin t)
+ (buffer-substring-no-properties begin end)))
   (org-element-create
'clock
(list :status status
@@ -4393,15 +4412,14 @@ Assume point is at the beginning of the timestamp."
 		  hour-start (nth 2 date)
 		  minute-start (nth 1 date
 	;; Compute date-end.  It can be provided directly in timestamp,
-	;; or extracted from time range.  Otherwise, it defaults to the
-	;; same values as date-start.
+	;; or extracted from time range.
 	(unless diaryp
 	  (let ((date (and date-end (org-parse-time-string date-end t
-	(setq year-end (or (nth 5 date) year-start)
-		  month-end (or (nth 4 date) month-start)
-		  day-end (or (nth 3 date) day-start)
-		  hour-end (or (nth 2 date) (car time-range) hour-start)
-		  minute-end (or (nth 1 date) (cdr time-range) minute-start
+	(setq year-end (or (nth 5 date) (and time-range year-start))
+		  month-end (or (nth 4 date) (and time-range month-start))
+		  

Re: Please document the caching and its user options

[Ihor Radchenko]

Daniel Clemente  writes:

> Thanks. I'm not sure about the "unless" part here:
>
>> Persisting the cache to disk […]
>> It is not recommended if the Org files
>> include sensitive data, unless the data is encrypted via `org-crypt'.")
>
> I first mentioned org-crypt because users of org-crypt may be
> surprised if they see encrypted data stored unencrypted in disk, due
> to this cache.

No unencrypted data should be stored in the cache _on fs_.
If it does get stored, it is a bug that should be reported.

> A user has somefile.org which contains some headers marked with the
> "crypt" tag. Only those headers are encrypted. The org-element cache
> may now cache the whole file, including the encrypted headers (this is
> ok). Now the user temporarily decrypts the encrypted header, works on
> it some time (including closing the file and opening it again) then
> encrypts the section again. During the time that the header was
> unencrypted, the org-element cache was storing information about
> unencrypted data in ~/.cache/org-persist, which could even be a remote
> server (NFS, SMB etc), not as private as the org file itself.

Nope. Storing to disk only happens when you kill the buffer and before
exiting Emacs. At that point, org-crypt must take care about
re-encrypting everything.

> The rest of the documentation change seems good, it improves things.
> I would just mention the shortcomings or disclaimers, if there are.
> For instance I worry about what may happen when different Emacs
> processes load the same Org files at the same time (e.g. I run several
> automated batch export jobs). And I guess that having a disk cache
> creates new problems, like when in a web browser a simple F5 won't
> refresh and you need S-F5.
> But if there are no shortcomings (i.e. all operations will always use
> up to date information and everything will keep working as usual when
> you enable on-disk cache), it's ok like it is. It's also good if it's
> explicitly mentioned. It could also be mentioned somewhere else, like
> in a cache section in the manual, if it gets one.

Multiple Emacs instances are handled correctly. I do not see much
point documenting that things are working as expected.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at .
Support Org development at ,
or support my work at 

Assigned: CVE-2024-39331 (was: [ANN] Emergency bugfix release: Org mode 9.7.5)

[Ihor Radchenko]

Ihor Radchenko  writes:

> emacs-orgm...@city17.xyz writes:
>
>> Will a CVE be released?
>
> Should be, I think.
> If nobody reports it independently by tomorrow, I will look into how to
> request a CVE number myself.

https://www.cve.org/CVERecord?id=CVE-2024-39331

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at .
Support Org development at ,
or support my work at 

Re: [ANN] Emergency bugfix release: Org mode 9.7.5

[Bastien Guerry]

Ihor Radchenko  writes:

> I just released Org mode 9.7.5 that fixes a critical vulnerability.
> The release is coordinated with emergency Emacs 29.4 release.

Thank you a lot for your diligent and careful work on this!

-- 
 Bastien Guerry

Re: ox-icalendar: Filter todo-types

[Ihor Radchenko]

Jack Kamm  writes:

> Perhaps these variables could map tags as well as todo keywords to
> status? E.g., in the following:
>
> (setq org-icalendar-event-status-map
>   '((cancelled . ("KILLED" "cancelled"

Then, may as well just map generalized property:

'((canceled . (("TODO" . "KILLED") ("TAGS" . "cancelled") ("PROPERTY" . 
"VALUE"

We might also allow setting something like

* An event that was canceled
:PROPERTIES:
:ICALENDAR_VEVENT_STATUS: canceled
:END:

  A non-todo entry with active timestamp <2024-06-23>.
  It will be exported with STATUS as CANCELLED.

Or, if we want to go even further, may allow TAGS-matcher
syntax/arbitrary function as the criterion.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at .
Support Org development at ,
or support my work at 

Re: ox-icalendar: Filter todo-types

[Jack Kamm]

Ihor Radchenko  writes:

> Another concern is that we already have `org-export-with-tasks' where
> you can specify which todo keywords should be exported and which
> headings should be exported, according to their todo keyword. So,
> "no-export" appears to unnecessary.

That's a good point. Michael, does `org-export-with-tasks' suffice for
your original need/request?

> However, after I looked at the RFC in more details now, I can see that
> the values of STATUS property depend on the entry type:
>
>statvalue-event = "TENTATIVE";Indicates event is tentative.
>/ "CONFIRMED";Indicates event is definite.
>/ "CANCELLED";Indicates event was cancelled.
>;Status values for a "VEVENT"
>
>statvalue-todo  = "NEEDS-ACTION" ;Indicates to-do needs action.
>/ "COMPLETED";Indicates to-do completed.
>/ "IN-PROCESS"   ;Indicates to-do in process of.
>/ "CANCELLED";Indicates to-do was cancelled.
>;Status values for "VTODO".
>
>statvalue-jour  = "DRAFT";Indicates journal is draft.
>/ "FINAL";Indicates journal is final.
>/ "CANCELLED";Indicates journal is removed.
>   ;Status values for "VJOURNAL".
>
> Maybe we can introduce separate variables mapping todo keyword to status
> depending on the entry type (VTODO vs. VEVENT; we do not export VJOURNAL)?

Sure, but we should also consider the STATUS for VEVENTs created from
non-TODO entries.

Perhaps these variables could map tags as well as todo keywords to
status? E.g., in the following:

(setq org-icalendar-event-status-map
  '((cancelled . ("KILLED" "cancelled"

Then any VEVENT created from entries whose todo-keyword or tag matches
the above would have STATUS set to CANCELLED. For example, both of the
following entries would contain VEVENTs exported as such:

* An event that was cancelled :cancelled:

  A non-todo entry with active timestamp <2024-06-23>.
  It will be exported with STATUS as CANCELLED.

* KILLED A todo that was cancelled
  SCHEDULED: <2024-06-22>

  This todo entry has both a scheduling timestamp as well as an active
  timestamp <2024-06-23>, and may create both VEVENT and VTODO. Any
  VEVENTs created will have STATUS as CANCELLED, due to "KILLED" being
  in org-icalendar-event-status-map.  Any VTODO created will have STATUS
  set according to org-icalendar-todo-status-map instead.